kola: add --no-ignition flag to run tests without ignition#4469
kola: add --no-ignition flag to run tests without ignition#4469yasminvalim wants to merge 1 commit intocoreos:mainfrom
Conversation
|
Skipping CI for Draft Pull Request. |
![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
This pull request introduces a --no-ignition flag to kola to allow running tests on pre-baked QCOW2 images, optimizing certain testing scenarios. However, the implementation of the --ssh-user flag introduces a potential SSH configuration injection vulnerability in mantle/platform/cluster.go as the user-supplied SSHUser string is written directly into the ssh-config file without sanitization. It is recommended to sanitize this input to prevent arbitrary SSH option injection. Additionally, consider improving the readability of the machine creation logic in the QEMU platform code.
| if bc.rconf.SSHUser != "" { | ||
| if _, err := fmt.Fprintf(sshBuf, " User %s\n", bc.rconf.SSHUser); err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
The SSHUser command-line flag is written directly into the ssh-config file without sanitization. An attacker who can control the command-line arguments to kola can inject arbitrary SSH configuration options by including newlines in the SSHUser string. This can lead to arbitrary command execution if the ssh-config file is used by the user or another tool (e.g., via ProxyCommand).
| qc.mu.Lock() | ||
|
|
||
| conf, err := qc.RenderUserData(userdata, map[string]string{}) | ||
| if err != nil { | ||
| noIgnition := qc.RuntimeConf().NoIgnition | ||
| var conf *conf.Conf | ||
| var confPath string | ||
| var err error | ||
| if noIgnition { | ||
|
|
||
| qc.mu.Unlock() | ||
| return nil, err | ||
| } else { | ||
| conf, err = qc.RenderUserData(userdata, map[string]string{}) | ||
| if err != nil { | ||
| qc.mu.Unlock() | ||
| return nil, err | ||
| } | ||
| qc.mu.Unlock() | ||
|
|
||
| if conf.IsIgnition() { | ||
| confPath = filepath.Join(dir, "ignition.json") | ||
| if err := conf.WriteFile(confPath); err != nil { | ||
| return nil, err | ||
| } | ||
| } else if !conf.IsEmpty() { | ||
| return nil, fmt.Errorf("qemu only supports Ignition or empty configs") | ||
| } | ||
| } |
There was a problem hiding this comment.
This block for handling Ignition is a bit complex and hard to follow due to the locking and branching. It can be simplified by restructuring the if condition and moving the lock to be more tightly scoped around the operation it protects. This will improve readability and maintainability.
noIgnition := qc.RuntimeConf().NoIgnition
var conf *conf.Conf
var confPath string
var err error
if !noIgnition {
qc.mu.Lock()
conf, err = qc.RenderUserData(userdata, map[string]string{})
qc.mu.Unlock()
if err != nil {
return nil, err
}
if conf.IsIgnition() {
confPath = filepath.Join(dir, "ignition.json")
if err := conf.WriteFile(confPath); err != nil {
return nil, err
}
} else if !conf.IsEmpty() {
return nil, fmt.Errorf("qemu only supports Ignition or empty configs")
}
}
Do you have any context for all of this? Running nested container images inside kubernetes/openshift (where we run our pipeline today) isn't trivial so I'm not sure if it will save us much. Also, the description here is contradictory. It says we should be able to run tests against a container, but then you mention a QCOW with an ssh key inject, which is a VM. What's the real goal here? |
Hey Dusty, I’ll send over the task and the context I have. To be honest, I’m still figuring it out myself. Since this is a spike, the goal is to investigate and see what’s actually feasible. The DoD in the jira ticket is to create a POC and document the different approaches. |
2 participants
TASK:
Running tests that not need ignition against a container image, instead of starting a VM, would drastically reduce the load on the Jenkins infra.
IDEA:
Adapt kola to be able to run tests without relying on ignition