Shell runner tasks #113
Shell runner tasks #113
Conversation
rgolangh
force-pushed
the
add_shell_tasks
branch
4 times, most recently
from
af5b713
to
0a82176
Compare
| }; | ||
| this.cmdline.add(command); | ||
| if (workContext.get("args") != null) { | ||
| this.cmdline.addAll(List.of(workContext.get("args").toString().split(" "))); |
There was a problem hiding this comment.
Is this not a security issue? A user can submit a command that scales privilege, no? like sh - sudo rm
There was a problem hiding this comment.
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
)
There was a problem hiding this comment.
@lshannon any input here?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| throw new RuntimeException(e); | ||
| } | ||
| finally { | ||
| tmpDir.delete(); |
There was a problem hiding this comment.
If you return in the try-and-catch, this is never going to be executed, no?
There was a problem hiding this comment.
that is exactly what finally is for. It is always executed, unless there's a System.exit statement
| underTest.outputConsumer = output::append; | ||
|
|
||
| assertThat(underTest.execute(ctx).getStatus()).isEqualTo(WorkStatus.COMPLETED); | ||
| assertThat(output.toString()).isEqualTo(randomUUID.toString()); |
There was a problem hiding this comment.
could we assert that the temp folder is deleted?
There was a problem hiding this comment.
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 .
rgolangh
force-pushed
the
add_shell_tasks
branch
from
0a82176
to
55a7605
Compare
|
/hold |
|
/hold cancel |
Introduce basic shell tasks that consumes {"command": "", "args": ""}
executes, and timeouts after a default of 10 minutes.
Pass "timeoutInSeconds": [timeout in seconds] to override
Signed-off-by: Roy Golan <rgolan@redhat.com>
rgolangh
force-pushed
the
add_shell_tasks
branch
from
55a7605
to
b19ff60
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rgolangh The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
|
@eloycoto do you think it is ready? |
|
The lgtm label should have been applied. Not sure why its still saying its not. Going to go ahead and merge this |
|
@rgolangh This code should be located in |
Introduce basic shell tasks that consumes {"command": "", "args": ""}
executes, and timeouts after a default of 10 minutes.
Pass "timeoutInSeconds": [timeout in seconds] to override
Signed-off-by: Roy Golan rgolan@redhat.com
FLPATH-162
Related-to: #123