Re: [pve-devel] [PATCH container 1/2] add old config and unprivileged to check_ct_modify_config_perm

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Tue, Aug 03, 2021 at 02:29:51PM +0200, Dominik Csapak wrote:
> we'll need that for checking the features more granularly
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PVE/API2/LXC.pm        | 6 ++++--
>  src/PVE/API2/LXC/Config.pm | 9 ++++++---
>  src/PVE/LXC.pm             | 2 +-
>  3 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> index b929481..e16ce6c 100644
> --- a/src/PVE/API2/LXC.pm
> +++ b/src/PVE/API2/LXC.pm
> @@ -254,7 +254,7 @@ __PACKAGE__->register_method({
>  	my $ostemplate = extract_param($param, 'ostemplate');
>  	my $storage = extract_param($param, 'storage') // 'local';
>  
> -	PVE::LXC::check_ct_modify_config_perm($rpcenv, $authuser, $vmid, $pool, $param, []);
> +	PVE::LXC::check_ct_modify_config_perm($rpcenv, $authuser, $vmid, $pool, undef, $param, [], $unprivileged);
>  
>  	my $storage_cfg = cfs_read_file("storage.cfg");
>  
> @@ -1679,7 +1679,9 @@ __PACKAGE__->register_method({
>  
>  	die "no options specified\n" if !scalar(keys %$param);
>  
> -	PVE::LXC::check_ct_modify_config_perm($rpcenv, $authuser, $vmid, undef, $param, []);
> +	my $conf = PVE::LXC::Config->load_config($vmid);
> +
> +	PVE::LXC::check_ct_modify_config_perm($rpcenv, $authuser, $vmid, undef, $conf, $param, [], $conf->{unprivileged});

3 lines down we have a locked section where we load the config and use
it for a digest check, so if we need the config for the check already,
we should probably move the check itself down into the locked section
now.
Alternatively with this being the resize API call which shouldn't affect the config much,
we could also add a comment and explicitly *not* pass the old config,
but that's probably not as nice.

>  
>  	my $storage_cfg = cfs_read_file("storage.cfg");
>  
> diff --git a/src/PVE/API2/LXC/Config.pm b/src/PVE/API2/LXC/Config.pm
> index 73fec36..ab136c0 100644
> --- a/src/PVE/API2/LXC/Config.pm
> +++ b/src/PVE/API2/LXC/Config.pm
> @@ -135,6 +135,9 @@ __PACKAGE__->register_method({
>  	my $node = extract_param($param, 'node');
>  	my $vmid = extract_param($param, 'vmid');
>  
> +	my $conf = PVE::LXC::Config->load_config($vmid);

Similar issue as above, but with a lot more work happening in between.
AFAICT it's nothing too expensive though, so we could extend the locked
section here, too. Alternatively remember this digest and add a 2nd
digest check in the locked section below further down.

> +	my $unprivileged = $conf->{unprivileged};
> +
>  	my $digest = extract_param($param, 'digest');
>  
>  	die "no options specified\n" if !scalar(keys %$param);
> @@ -144,8 +147,8 @@ __PACKAGE__->register_method({
>  	my $revert_str = extract_param($param, 'revert');
>  	my @revert = PVE::Tools::split_list($revert_str);
>  
> -	PVE::LXC::check_ct_modify_config_perm($rpcenv, $authuser, $vmid, undef, {}, [@delete]);
> -	PVE::LXC::check_ct_modify_config_perm($rpcenv, $authuser, $vmid, undef, {}, [@revert]);
> +	PVE::LXC::check_ct_modify_config_perm($rpcenv, $authuser, $vmid, undef, $conf, {}, [@delete], $unprivileged);
> +	PVE::LXC::check_ct_modify_config_perm($rpcenv, $authuser, $vmid, undef, $conf, {}, [@revert], $unprivileged);
>  
>  	foreach my $opt (@revert) {
>  	    raise_param_exc({ revert => "unknown option '$opt'" })
> @@ -166,7 +169,7 @@ __PACKAGE__->register_method({
>  		if grep(/^$opt$/, @revert);
>  	}
>  
> -	PVE::LXC::check_ct_modify_config_perm($rpcenv, $authuser, $vmid, undef, $param, []);
> +	PVE::LXC::check_ct_modify_config_perm($rpcenv, $authuser, $vmid, undef, $conf, $param, [], $unprivileged);
>  
>  	my $storage_cfg = PVE::Storage::config();
>  
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index 139f901..32a2127 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -1242,7 +1242,7 @@ sub template_create {
>  }
>  
>  sub check_ct_modify_config_perm {
> -    my ($rpcenv, $authuser, $vmid, $pool, $newconf, $delete) = @_;
> +    my ($rpcenv, $authuser, $vmid, $pool, $oldconf, $newconf, $delete, $unprivileged) = @_;
>  
>      return 1 if $authuser eq 'root@pam';
>      my $storage_cfg = PVE::Storage::config();
> -- 
> 2.30.2