[pve-devel] [PATCH container/manager] default nesting for unpriv containers in ui

[pve-devel] [PATCH container/manager] default nesting for unpriv containers in ui

[Dominik Csapak]

since many modern containers need the nesting feature to work properly
(thanks systemd...), we add a checkbox that is on by default
(and disables with unprivileged, since nested privileged containers
are not very secure)

to do that, we first have to loosen the nesting constraints in the api
a bit. we do that by allowing to set that for unprivileged containers
when the user has the 'VM.Allocate' privilege.

(just to note: a user with that right can also create privileged
containers, but could not enable nesting for them)

pve-container:

Dominik Csapak (2):
  add old config and unprivileged to check_ct_modify_config_perm
  allow nesting to be changed for VM.Allocate on unprivileged containers

 src/PVE/API2/LXC.pm        |  6 +++--
 src/PVE/API2/LXC/Config.pm |  9 +++++---
 src/PVE/LXC.pm             | 45 +++++++++++++++++++++++++++++++++++---
 3 files changed, 52 insertions(+), 8 deletions(-)

pve-manager:

Dominik Csapak (2):
  ui: lxc/Options: allow opening features window for VM.Allocate
  ui: lxc/CreateWizard: add a 'nesting' checkbox and enable it by
    default

 www/manager6/lxc/CreateWizard.js | 10 ++++++++++
 www/manager6/lxc/Options.js      |  2 +-
 2 files changed, 11 insertions(+), 1 deletion(-)

-- 
2.30.2

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

[Dominik Csapak]

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});
 
 	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);
+	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

[pve-devel] [PATCH container 2/2] allow nesting to be changed for VM.Allocate on unprivileged containers

[Dominik Csapak]

instead of it being root only

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/LXC.pm | 43 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 41 insertions(+), 2 deletions(-)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 32a2127..abe8ac3 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -1270,8 +1270,47 @@ sub check_ct_modify_config_perm {
 		 $opt eq 'searchdomain' || $opt eq 'hostname') {
 	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.Network']);
 	} elsif ($opt eq 'features') {
-	    # For now this is restricted to root@pam
-	    raise_perm_exc("changing feature flags is only allowed for root\@pam");
+	    raise_perm_exc("changing feature flags for privileged container is only allowed for root\@pam")
+		if !$unprivileged;
+
+	    my $nesting_changed = 0;
+	    my $other_changed = 0;
+	    if (!$delete) {
+		my $features = PVE::LXC::Config->parse_features($newconf->{$opt});
+		if (defined($oldconf) && $oldconf->{$opt}) {
+		    # existing container with features
+		    my $old_features = PVE::LXC::Config->parse_features($oldconf->{$opt});
+		    for my $feature ((keys %$old_features, keys %$features)) {
+			if ($old_features->{$feature} ne $features->{$feature}) {
+			    if ($feature eq 'nesting') {
+				$nesting_changed = 1;
+				next;
+			    } else {
+				$other_changed = 1;
+				last;
+			    }
+			}
+		    }
+		} else {
+		    # new container or no features defined
+		    if (scalar(keys %$features) == 1 && $features->{nesting}) {
+			$nesting_changed = 1;
+		    } elsif (scalar(keys %$features) > 0) {
+			$other_changed = 1;
+		    }
+		}
+	    } else {
+		my $features = PVE::LXC::Config->parse_features($oldconf->{$opt});
+		if (scalar(keys %$features) == 1 && $features->{nesting}) {
+		    $nesting_changed = 1;
+		} elsif (scalar(keys %$features) > 0) {
+		    $other_changed = 1;
+		}
+	    }
+	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Allocate'])
+		if $nesting_changed;
+	    raise_perm_exc("changing feature flags (except nesting) is only allowed for root\@pam")
+		if $other_changed;
 	} elsif ($opt eq 'hookscript') {
 	    # For now this is restricted to root@pam
 	    raise_perm_exc("changing the hookscript is only allowed for root\@pam");
-- 
2.30.2

[pve-devel] [PATCH manager 1/2] ui: lxc/Options: allow opening features window for VM.Allocate

[Dominik Csapak]

since VM.Allocate can at least change the nesting value

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/manager6/lxc/Options.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/www/manager6/lxc/Options.js b/www/manager6/lxc/Options.js
index b64d03a9..f2661dfc 100644
--- a/www/manager6/lxc/Options.js
+++ b/www/manager6/lxc/Options.js
@@ -136,7 +136,7 @@ Ext.define('PVE.lxc.Options', {
 	    features: {
 		header: gettext('Features'),
 		defaultValue: Proxmox.Utils.noneText,
-		editor: Proxmox.UserName === 'root@pam'
+		editor: Proxmox.UserName === 'root@pam' || caps.vms['VM.Allocate']
 		    ? 'PVE.lxc.FeaturesEdit' : undefined,
 	    },
 	    hookscript: {
-- 
2.30.2

[pve-devel] [PATCH manager 2/2] ui: lxc/CreateWizard: add a 'nesting' checkbox and enable it by default

[Dominik Csapak]

but only enable the field for unprivileged containers.
We do this, since newer containers need this feature for basic
functions.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/manager6/lxc/CreateWizard.js | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/www/manager6/lxc/CreateWizard.js b/www/manager6/lxc/CreateWizard.js
index bd84dece..aead515f 100644
--- a/www/manager6/lxc/CreateWizard.js
+++ b/www/manager6/lxc/CreateWizard.js
@@ -62,6 +62,16 @@ Ext.define('PVE.lxc.CreateWizard', {
 		    },
 		    fieldLabel: gettext('Unprivileged container'),
 		},
+		{
+		    xtype: 'proxmoxcheckbox',
+		    name: 'features',
+		    inputValue: 'nesting=1',
+		    value: true,
+		    bind: {
+			disabled: '{!unprivileged}',
+		    },
+		    fieldLabel: gettext('Nesting'),
+		},
 	    ],
 	    column2: [
 		{
-- 
2.30.2

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

[Wolfgang Bumiller]

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

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

[Fabian Ebner]

Am 03.08.21 um 14:29 schrieb Dominik Csapak:
> 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});
>   
>   	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);
> +	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);

When restoring, $param can be {} here.

This means together with the other patches, it's possible as non-root to:
1. Enable nesting on an unprivileged container
2. Backup
3. Restore as privileged, and nesting is still set

>   
>   	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();
> 

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

[Fabian Ebner]

Am 04.08.21 um 10:47 schrieb Fabian Ebner:
> Am 03.08.21 um 14:29 schrieb Dominik Csapak:
>> 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);

Sorry, I meant here ;)

>>       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});
>>       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);
>> +    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);
> 
> When restoring, $param can be {} here.
> 
> This means together with the other patches, it's possible as non-root to:
> 1. Enable nesting on an unprivileged container
> 2. Backup
> 3. Restore as privileged, and nesting is still set
> 
>>       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();
>>
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 

Re: [pve-devel] [PATCH container 2/2] allow nesting to be changed for VM.Allocate on unprivileged containers

[Wolfgang Bumiller]

On Tue, Aug 03, 2021 at 02:29:52PM +0200, Dominik Csapak wrote:
> instead of it being root only
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PVE/LXC.pm | 43 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 41 insertions(+), 2 deletions(-)
> 
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index 32a2127..abe8ac3 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -1270,8 +1270,47 @@ sub check_ct_modify_config_perm {
>  		 $opt eq 'searchdomain' || $opt eq 'hostname') {
>  	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.Network']);
>  	} elsif ($opt eq 'features') {
> -	    # For now this is restricted to root@pam
> -	    raise_perm_exc("changing feature flags is only allowed for root\@pam");
> +	    raise_perm_exc("changing feature flags for privileged container is only allowed for root\@pam")
> +		if !$unprivileged;
> +
> +	    my $nesting_changed = 0;
> +	    my $other_changed = 0;
> +	    if (!$delete) {
> +		my $features = PVE::LXC::Config->parse_features($newconf->{$opt});
> +		if (defined($oldconf) && $oldconf->{$opt}) {
> +		    # existing container with features
> +		    my $old_features = PVE::LXC::Config->parse_features($oldconf->{$opt});
> +		    for my $feature ((keys %$old_features, keys %$features)) {
> +			if ($old_features->{$feature} ne $features->{$feature}) {
> +			    if ($feature eq 'nesting') {
> +				$nesting_changed = 1;
> +				next;
> +			    } else {
> +				$other_changed = 1;
> +				last;

Breaking out here means you *tend* to prefer the `other_changed` error, but...

> +			    }
> +			}
> +		    }
> +		} else {
> +		    # new container or no features defined
> +		    if (scalar(keys %$features) == 1 && $features->{nesting}) {
> +			$nesting_changed = 1;
> +		    } elsif (scalar(keys %$features) > 0) {
> +			$other_changed = 1;
> +		    }
> +		}
> +	    } else {
> +		my $features = PVE::LXC::Config->parse_features($oldconf->{$opt});
> +		if (scalar(keys %$features) == 1 && $features->{nesting}) {

...here you prefer the nesting check, but only if nothing else
changes alongside it...

> +		    $nesting_changed = 1;
> +		} elsif (scalar(keys %$features) > 0) {
> +		    $other_changed = 1;
> +		}
> +	    }
> +	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Allocate'])
> +		if $nesting_changed;

...but hash key randomization may randomly give you $nesting_changed=1
first and cause this message to appear instead of the one below.

So we should probably swap the two checks for consistency?

> +	    raise_perm_exc("changing feature flags (except nesting) is only allowed for root\@pam")
> +		if $other_changed;
>  	} elsif ($opt eq 'hookscript') {
>  	    # For now this is restricted to root@pam
>  	    raise_perm_exc("changing the hookscript is only allowed for root\@pam");
> -- 
> 2.30.2

Re: [pve-devel] [PATCH container 2/2] allow nesting to be changed for VM.Allocate on unprivileged containers

[Fabian Ebner]

Am 03.08.21 um 14:29 schrieb Dominik Csapak:
> instead of it being root only
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>   src/PVE/LXC.pm | 43 +++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 41 insertions(+), 2 deletions(-)
> 
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index 32a2127..abe8ac3 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -1270,8 +1270,47 @@ sub check_ct_modify_config_perm {
>   		 $opt eq 'searchdomain' || $opt eq 'hostname') {
>   	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.Network']);
>   	} elsif ($opt eq 'features') {
> -	    # For now this is restricted to root@pam
> -	    raise_perm_exc("changing feature flags is only allowed for root\@pam");
> +	    raise_perm_exc("changing feature flags for privileged container is only allowed for root\@pam")
> +		if !$unprivileged;
> +
> +	    my $nesting_changed = 0;
> +	    my $other_changed = 0;
> +	    if (!$delete) {
> +		my $features = PVE::LXC::Config->parse_features($newconf->{$opt});
> +		if (defined($oldconf) && $oldconf->{$opt}) {
> +		    # existing container with features
> +		    my $old_features = PVE::LXC::Config->parse_features($oldconf->{$opt});
> +		    for my $feature ((keys %$old_features, keys %$features)) {
> +			if ($old_features->{$feature} ne $features->{$feature}) {

Produces a warning when only one is not undef:
     Use of uninitialized value in string ne

> +			    if ($feature eq 'nesting') {
> +				$nesting_changed = 1;
> +				next;
> +			    } else {
> +				$other_changed = 1;
> +				last;
> +			    }
> +			}
> +		    }
> +		} else {
> +		    # new container or no features defined
> +		    if (scalar(keys %$features) == 1 && $features->{nesting}) {
> +			$nesting_changed = 1;
> +		    } elsif (scalar(keys %$features) > 0) {
> +			$other_changed = 1;
> +		    }
> +		}
> +	    } else {
> +		my $features = PVE::LXC::Config->parse_features($oldconf->{$opt});
> +		if (scalar(keys %$features) == 1 && $features->{nesting}) {
> +		    $nesting_changed = 1;
> +		} elsif (scalar(keys %$features) > 0) {
> +		    $other_changed = 1;
> +		}
> +	    }
> +	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Allocate'])
> +		if $nesting_changed;
> +	    raise_perm_exc("changing feature flags (except nesting) is only allowed for root\@pam")
> +		if $other_changed;
>   	} elsif ($opt eq 'hookscript') {
>   	    # For now this is restricted to root@pam
>   	    raise_perm_exc("changing the hookscript is only allowed for root\@pam");
>