[pve-devel] [PATCH manager 2/4] api: new parameter for each shell to pass custom command string

[Fabian Grünbichler]

On Thu, Jan 10, 2019 at 01:54:31PM +0100, Tim Marx wrote:
> Signed-off-by: Tim Marx <t.marx at proxmox.com>
> ---
>  PVE/API2/Nodes.pm | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
> index dd5471f8..c1f73cd0 100644
> --- a/PVE/API2/Nodes.pm
> +++ b/PVE/API2/Nodes.pm
> @@ -658,6 +658,11 @@ __PACKAGE__->register_method ({
>  		optional => 1,
>  		default => 0,
>  	    },
> +	    cmd => {
> +		type => 'string',
> +		description => "Run command instead of normal shell.",
> +		optional => 1,

I'd prefer an enum/whitelist approach here, which would also allow us to
deprecate the 'upgrade' parameter in favor of this.

e.g.

enum => ['upgrade', 'ceph']

this makes the interface way less confusing (it's clear that all these
are mutually exclusive, even if we add seven additional commands) and
does not open up the possibility of directly executing arbitrary user
provided commands via a forged request in case we ever have a bug of
that sort.

> +	    },
>  	    websocket => {
>  		optional => 1,
>  		type => 'boolean',
> @@ -731,6 +736,8 @@ __PACKAGE__->register_method ({
>  		my $upgradecmd = "pveupgrade --shell";
>  		$upgradecmd = PVE::Tools::shellquote($upgradecmd) if $remip;
>  		$shcmd = [ '/bin/bash', '-c', $upgradecmd ];
> +	    } elsif ($param->{cmd}) {
> +		push(@$shcmd, split(' ',$param->{cmd}));

this is wrong - what about spaces inside quoted strings? the whitelist
approach avoids this altogether since we control the commands that each
whitelist entry gets mapped to.

>  	    } else {
>  		$shcmd = [ '/bin/login', '-f', 'root' ];
>  	    }
> @@ -817,6 +824,11 @@ __PACKAGE__->register_method ({
>  		optional => 1,
>  		default => 0,
>  	    },
> +	    cmd => {
> +		type => 'string',
> +		description => "Run command instead of normal shell.",
> +		optional => 1,

as above - and since this parameter gets used more than once, it
probably makes sense to define a 'shellcmd' standard option somewhere
once and reuse it for all these API paths.

(same would also apply to the 'upgrade' parameter that already exists)

> +	    },
>  	},
>      },
>      returns => {
> @@ -861,6 +873,8 @@ __PACKAGE__->register_method ({
>  	if ($user eq 'root at pam') {
>  	    if ($param->{upgrade}) {
>  		$concmd = [ '/usr/bin/pveupgrade', '--shell' ];
> +	    } elsif ($param->{cmd}) {
> +		@$concmd = split(' ',$param->{cmd});

as above

>  	    } else {
>  		$concmd = [ '/bin/login', '-f', 'root' ];
>  	    }
> @@ -964,6 +978,11 @@ __PACKAGE__->register_method ({
>  		optional => 1,
>  		default => 0,
>  	    },
> +	    cmd => {
> +		type => 'string',
> +		description => "Run command instead of normal shell.",
> +		optional => 1,

as above

> +	    },
>  	},
>      },
>      returns => get_standard_option('remote-viewer-config'),
> @@ -991,6 +1010,8 @@ __PACKAGE__->register_method ({
>  	    if ($param->{upgrade}) {
>  		my $upgradecmd = "pveupgrade --shell";
>  		$shcmd = [ '/bin/bash', '-c', $upgradecmd ];
> +	    } elsif ($param->{cmd}) {
> +		@$shcmd = split(' ',$param->{cmd});

as above

>  	    } else {
>  		$shcmd = [ '/bin/login', '-f', 'root' ];
>  	    }
> -- 
> 2.11.0
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel