Skip to content
This repository has been archived by the owner on Jul 23, 2024. It is now read-only.

Shell runner tasks #113

Merged
merged 1 commit into from
Mar 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions parodos-model-api/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,12 @@
<version>${junit.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.assertj</groupId>
<artifactId>assertj-core</artifactId>
<version>3.19.0</version>
<scope>test</scope>
</dependency>
</dependencies>

<build>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
package com.redhat.parodos.workflow.task.shell;

import com.redhat.parodos.workflow.task.BaseWorkFlowTask;
import com.redhat.parodos.workflows.work.DefaultWorkReport;
import com.redhat.parodos.workflows.work.WorkContext;
import com.redhat.parodos.workflows.work.WorkReport;
import com.redhat.parodos.workflows.work.WorkStatus;
import lombok.extern.slf4j.Slf4j;

import java.io.*;
import java.nio.file.Files;
import java.nio.file.attribute.PosixFilePermissions;
import java.util.*;
import java.util.concurrent.TimeUnit;
import java.util.function.Consumer;

import static java.nio.file.attribute.PosixFilePermission.*;

/**
* ShellRunnerTask takes a script and an optional env and runs them to completion and
* returns the exit code. For exit code 0 the return status should be successful and for
* anything else it should be a failure.
*/
@Slf4j
public class ShellRunnerTask extends BaseWorkFlowTask {

public static final int DEFAULT_EXECUTION_TIMEOUT_IN_SECONDS = 600;

Consumer<String> outputConsumer = log::info;

private static class ShellRunnerTaskParams {

/**
* cmdline to execute including arguments, must contain at least one string
*/
ArrayList<String> cmdline;

int timeoutInSeconds = DEFAULT_EXECUTION_TIMEOUT_IN_SECONDS;

ShellRunnerTaskParams(WorkContext workContext) {
String command = (String) workContext.get("command");
if (command == null || command.isBlank()) {
throw new IllegalArgumentException("argument 'command' is empty or blank");
}
this.cmdline = new ArrayList<>() {
};
this.cmdline.add(command);
if (workContext.get("args") != null) {
this.cmdline.addAll(List.of(workContext.get("args").toString().split(" ")));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this not a security issue? A user can submit a command that scales privilege, no? like sh - sudo rm

Copy link
Contributor Author

@rgolangh rgolangh Mar 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very very good point. The thing is that I really don't know what the deployment model is yet. For example, will the user the jvm runs under is in sudoers at all? could we actually ensure podman is part of the deployment and so run every execution part of a ubi9/micro image? (like podman run ubi9/micro $cmd $args
)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lshannon any input here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, and also, I would run at least in a separate shell, like sh 'command', if not can get the ENV variables and maybe connect to the database itself.

TBH, I would move this to "container shell command", and be able to specify the image and the command to execute, for me, it will be 10000 times safer.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need to work on deployment story and we can make sure that ability to exploit this issue is limited or not possible. Let's discuss this issue while working on FLPATH-198.

}
if (workContext.get("timeoutInSeconds") != null) {
this.timeoutInSeconds = Integer.parseInt(workContext.get("timeoutInSeconds").toString());
}
}

}

@Override
public WorkReport execute(WorkContext workContext) {
var params = new ShellRunnerTaskParams(workContext);
var pb = new ProcessBuilder(params.cmdline);
File tmpDir = null;
try {
tmpDir = Files
.createTempDirectory("parodos-shelltask-runner",
PosixFilePermissions.asFileAttribute(EnumSet.of(OWNER_READ, OWNER_WRITE, OWNER_EXECUTE)))
.toFile();
pb.directory(tmpDir);
pb.redirectErrorStream(true);

log.info("begin shell task invocation");
Process p = pb.start();
var elapsed = !p.waitFor(params.timeoutInSeconds, TimeUnit.SECONDS);
if (elapsed) {
// timeouts, assume incomplete because we can't read the exit code
return new DefaultWorkReport(WorkStatus.FAILED, workContext);
}
// read output only if we finished waiting otherwise the thread
// waits till the end of execution.
try (var r = new BufferedReader(new InputStreamReader(p.getInputStream()))) {
r.lines().forEach(outputConsumer);
}
log.info("ended shell task invocation");
return new DefaultWorkReport(p.exitValue() == 0 ? WorkStatus.COMPLETED : WorkStatus.FAILED, workContext);
}
catch (IOException | InterruptedException e) {
throw new RuntimeException(e);
}
finally {
if (tmpDir != null) {
tmpDir.delete();
}
}
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
package com.redhat.parodos.workflow.task.shell;

import com.redhat.parodos.workflows.work.WorkContext;
import com.redhat.parodos.workflows.work.WorkStatus;
import org.junit.Before;
import org.junit.Test;

import java.util.UUID;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;

public class ShellRunnerTaskTest {

ShellRunnerTask underTest;

WorkContext ctx;

@Before
public void setUp() {
underTest = new ShellRunnerTask();
ctx = new WorkContext();
}

@Test
public void executeWithOutput() {
UUID randomUUID = UUID.randomUUID();
ctx.put("command", "echo");
ctx.put("args", randomUUID);
var output = new StringBuilder();
underTest.outputConsumer = output::append;

assertThat(underTest.execute(ctx).getStatus()).isEqualTo(WorkStatus.COMPLETED);
assertThat(output.toString()).isEqualTo(randomUUID.toString());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we assert that the temp folder is deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

following the comment above, we can count on the jvm to execute that. but for extra cation we can parametrize the call to pass in a temp dir ... wonder if its worth it.
Another though I had - writing stuff into /tmp costs memory while /var/tmp is disk space only and I tend to think those executions could involve a lot of stuff written into the scratch space of the task .

}

@Test
public void failsWhenTimeouts() {
ctx.put("command", "sleep");
ctx.put("args", "5s");
ctx.put("timeoutInSeconds", "1");
assertThat(underTest.execute(ctx).getStatus()).isEqualTo(WorkStatus.FAILED);
}

@Test
public void completesIfExitZero() {
ctx.put("command", "true");
assertThat(underTest.execute(ctx).getStatus()).isEqualTo(WorkStatus.COMPLETED);
}

@Test
public void failsIfExitNonZero() {
ctx.put("command", "false");
assertThat(underTest.execute(ctx).getStatus()).isEqualTo(WorkStatus.FAILED);
}

@Test()
public void failsOnMissingCommand() {
ctx.put("command", "");
assertThatExceptionOfType(IllegalArgumentException.class).isThrownBy(() -> underTest.execute(ctx));
}

}
7 changes: 7 additions & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,13 @@
<module>pattern-detection-library</module>
<module>coverage</module>
</modules>
<dependencies>
<dependency>
<groupId>org.assertj</groupId>
<artifactId>assertj-core</artifactId>
<version>3.24.2</version>
</dependency>
</dependencies>
<build>
<pluginManagement>
<plugins>
Expand Down