alces-software / adminware

A sandbox CLI for running commands remotely across nodes
1 stars 0 forks source link

Rework Arguments #163

Closed ColonelPanicks closed 5 years ago

ColonelPanicks commented 5 years ago

Currently, to my understanding, the arguments system passes anything from the end of the run command onto the end of the defined command.

This could lead to a lot of potential security risks through piping or potentially other ways so would it be possible to make arguments a definable feature. Such that the argument is enforced to be purely alphanumeric and can be "slotted in" to the command.

For example, if I had a batch report command for showing processes running for a user as follows:

command: "ps -u USER --no-headers |awk '{print $4}'"

I would like to be able to run it through adminware and simply provide a username to slot in there, perhaps like

run show-processes-for-user -g nodes myuser
DavidMarchant commented 5 years ago

Currently, as it stands, all arguments that are passed to the commands are escaped with shlex quote - https://docs.python.org/3/library/shlex.html (https://github.com/alces-software/adminware/blob/develop/src/models/job.py#L167)

This does, as far as I am aware, prevent all security risks with arguments however it brings its own problem to the current implementation - namely that many arguments passed to the commands will be 'mangled' silently before execution and thus a tool might not act as expected. (e.g. if a pipe is passed for whatever benign purpose) What are your thoughts on this solution?

For the second part of your comment can I just confirm what you're asking of us - the ability to enter arguments into a predefined section of the shell command, not necessarily at the end? USER in your example? If so I'm sure this would be do-able with some tinkering

WilliamMcCumstie commented 5 years ago

From discussions IRL:

The easiest way to do this is through environment variables. The config.yaml files should contain an arguments array of env var names. This can be picked up by the CLI and requested as input arguments. Before the command is ran, these env vars need to be exported to the environment.

This will allow the arguments to be substituted using $USER etc

@ColonelPanicks How does that sound? For the first pass all the listed arguments will be required. We can look at allowing optional arguments at a future date.

ColonelPanicks commented 5 years ago

Thanks for your response @DavidMarchant!

As William has mentioned it's worth using a var name sort of structure as this will hopefully translate into a fairly simple way of adding named arguments to a command.

What @WilliamMcCumstie has proposed sounds good, I'd be happy to go ahead with this.

WilliamMcCumstie commented 5 years ago

Update:

env vars shouldn't be used as they will be inherited by subshells. Instead, they should be set as local variables within the shell