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

[pve-devel] [PATCH container/manager v2] 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)

changes from v1:
* prevent comparing undefined $(old)features->{$features} by first
  extracting it into a variable with a fallback of '' and compare that
* reorder the permission checks so that they are returned consistently
* add patch that removes features when restoring an unprivileged
  container as privileged

pve-container:

Dominik Csapak (3):
  add old config and unprivileged to check_ct_modify_config_perm
  allow nesting to be changed for VM.Allocate on unprivileged containers
  skip features when restoring an unprivileged container as privileged

 src/PVE/API2/LXC.pm        |  6 +--
 src/PVE/API2/LXC/Config.pm | 95 +++++++++++++++++++-------------------
 src/PVE/LXC.pm             | 47 +++++++++++++++++--
 src/PVE/LXC/Create.pm      |  5 ++
 4 files changed, 100 insertions(+), 53 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 v2 1/3] add old config and unprivileged to check_ct_modify_config_perm

[Dominik Csapak]

we'll need that for checking the features more granularly
for it to work correctly, we have to move the permission checks
into the 'lock_config' sub, since we now also need to check the current
config and it could change between the permission check and the lock

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/API2/LXC.pm        |  6 +--
 src/PVE/API2/LXC/Config.pm | 95 +++++++++++++++++++-------------------
 src/PVE/LXC.pm             |  2 +-
 3 files changed, 52 insertions(+), 51 deletions(-)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index b929481..afef7ec 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,8 +1679,6 @@ __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 $storage_cfg = cfs_read_file("storage.cfg");
 
 	my $code = sub {
@@ -1688,6 +1686,8 @@ __PACKAGE__->register_method({
 	    my $conf = PVE::LXC::Config->load_config($vmid);
 	    PVE::LXC::Config->check_lock($conf);
 
+	    PVE::LXC::check_ct_modify_config_perm($rpcenv, $authuser, $vmid, undef, $conf, $param, [], $conf->{unprivileged});
+
 	    PVE::Tools::assert_if_modified($digest, $conf->{digest});
 
 	    my $running = PVE::LXC::check_running($vmid);
diff --git a/src/PVE/API2/LXC/Config.pm b/src/PVE/API2/LXC/Config.pm
index 73fec36..1fec048 100644
--- a/src/PVE/API2/LXC/Config.pm
+++ b/src/PVE/API2/LXC/Config.pm
@@ -144,62 +144,63 @@ __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]);
+	my $code = sub {
 
-	foreach my $opt (@revert) {
-	    raise_param_exc({ revert => "unknown option '$opt'" })
-		if !PVE::LXC::Config->option_exists($opt);
+	    my $conf = PVE::LXC::Config->load_config($vmid);
+	    PVE::LXC::Config->check_lock($conf);
 
-	    raise_param_exc({ revert => "you can't use '-$opt' and '-revert $opt' at the same time" })
-		if defined($param->{$opt});
-	}
+	    PVE::Tools::assert_if_modified($digest, $conf->{digest});
 
-	foreach my $opt (@delete) {
-	    raise_param_exc({ delete => "unknown option '$opt'" })
-		if !PVE::LXC::Config->option_exists($opt);
+	    my $unprivileged = $conf->{unprivileged};
+	    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);
 
-	    raise_param_exc({ delete => "you can't use '-$opt' and -delete $opt' at the same time" })
-		if defined($param->{$opt});
+	    foreach my $opt (@revert) {
+		raise_param_exc({ revert => "unknown option '$opt'" })
+		    if !PVE::LXC::Config->option_exists($opt);
 
-	    raise_param_exc({ delete => "you can't use '-delete $opt' and '-revert $opt' at the same time" })
-		if grep(/^$opt$/, @revert);
-	}
+		raise_param_exc({ revert => "you can't use '-$opt' and '-revert $opt' at the same time" })
+		    if defined($param->{$opt});
+	    }
 
-	PVE::LXC::check_ct_modify_config_perm($rpcenv, $authuser, $vmid, undef, $param, []);
-
-	my $storage_cfg = PVE::Storage::config();
-
-	my $repl_conf = PVE::ReplicationConfig->new();
-	my $is_replicated = $repl_conf->check_for_existing_jobs($vmid, 1);
-	if ($is_replicated) {
-	    PVE::LXC::Config->foreach_volume($param, sub {
-		my ($opt, $mountpoint) = @_;
-		my $volid = $mountpoint->{volume};
-		return if !$volid || !($mountpoint->{replicate}//1);
-		if ($mountpoint->{type} eq 'volume') {
-		    my ($storeid, $format);
-		    if ($volid =~ $PVE::LXC::NEW_DISK_RE) {
-			$storeid = $1;
-			$format = $mountpoint->{format} || PVE::Storage::storage_default_format($storage_cfg, $storeid);
-		    } else {
-			($storeid, undef) = PVE::Storage::parse_volume_id($volid, 1);
-			$format = (PVE::Storage::parse_volname($storage_cfg, $volid))[6];
-		    }
-		    return if PVE::Storage::storage_can_replicate($storage_cfg, $storeid, $format);
-		    my $scfg = PVE::Storage::storage_config($storage_cfg, $storeid);
-		    return if $scfg->{shared};
-		}
-		die "cannot add non-replicatable volume to a replicated VM\n";
-	    });
-	}
+	    foreach my $opt (@delete) {
+		raise_param_exc({ delete => "unknown option '$opt'" })
+		    if !PVE::LXC::Config->option_exists($opt);
 
-	my $code = sub {
+		raise_param_exc({ delete => "you can't use '-$opt' and -delete $opt' at the same time" })
+		    if defined($param->{$opt});
 
-	    my $conf = PVE::LXC::Config->load_config($vmid);
-	    PVE::LXC::Config->check_lock($conf);
+		raise_param_exc({ delete => "you can't use '-delete $opt' and '-revert $opt' at the same time" })
+		    if grep(/^$opt$/, @revert);
+	    }
 
-	    PVE::Tools::assert_if_modified($digest, $conf->{digest});
+	    PVE::LXC::check_ct_modify_config_perm($rpcenv, $authuser, $vmid, undef, $conf, $param, [], $unprivileged);
+
+	    my $storage_cfg = PVE::Storage::config();
+
+	    my $repl_conf = PVE::ReplicationConfig->new();
+	    my $is_replicated = $repl_conf->check_for_existing_jobs($vmid, 1);
+	    if ($is_replicated) {
+		PVE::LXC::Config->foreach_volume($param, sub {
+		    my ($opt, $mountpoint) = @_;
+		    my $volid = $mountpoint->{volume};
+		    return if !$volid || !($mountpoint->{replicate}//1);
+		    if ($mountpoint->{type} eq 'volume') {
+			my ($storeid, $format);
+			if ($volid =~ $PVE::LXC::NEW_DISK_RE) {
+			    $storeid = $1;
+			    $format = $mountpoint->{format} || PVE::Storage::storage_default_format($storage_cfg, $storeid);
+			} else {
+			    ($storeid, undef) = PVE::Storage::parse_volume_id($volid, 1);
+			    $format = (PVE::Storage::parse_volname($storage_cfg, $volid))[6];
+			}
+			return if PVE::Storage::storage_can_replicate($storage_cfg, $storeid, $format);
+			my $scfg = PVE::Storage::storage_config($storage_cfg, $storeid);
+			return if $scfg->{shared};
+		    }
+		    die "cannot add non-replicatable volume to a replicated VM\n";
+		});
+	    }
 
 	    my $running = PVE::LXC::check_running($vmid);
 
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 v2 2/3] 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 | 45 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 43 insertions(+), 2 deletions(-)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 32a2127..dbdec23 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -1270,8 +1270,49 @@ 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)) {
+			my $old = $old_features->{$feature} // '';
+			my $new = $features->{$feature} // '';
+			if ($old ne $new) {
+			    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;
+		}
+	    }
+	    raise_perm_exc("changing feature flags (except nesting) is only allowed for root\@pam")
+		if $other_changed;
+	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Allocate'])
+		if $nesting_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 container v2 3/3] skip features when restoring an unprivileged container as privileged

[Dominik Csapak]

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

diff --git a/src/PVE/LXC/Create.pm b/src/PVE/LXC/Create.pm
index e8233b6..eea7c0d 100644
--- a/src/PVE/LXC/Create.pm
+++ b/src/PVE/LXC/Create.pm
@@ -320,6 +320,11 @@ sub sanitize_and_merge_config {
 	# storage supports creating a template there
 	next if $key =~ /^template$/;
 
+	if ($restricted && $key eq 'features' && !$conf->{unprivileged} && $oldconf->{unprivileged}) {
+	    warn "changing from unprivileged to privileged, skipping features\n";
+	    next;
+	}
+
 	if ($key eq 'lxc' && $restricted) {
 	    my $lxc_list = $oldconf->{'lxc'};
 	    warn "skipping custom lxc options, restore manually as root:\n";
-- 
2.30.2

[pve-devel] [PATCH manager v2 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 v2 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

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

[Wolfgang Bumiller]

applied series

On Wed, Aug 04, 2021 at 12:51:06PM +0200, Dominik Csapak wrote:
> 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)
> 
> changes from v1:
> * prevent comparing undefined $(old)features->{$features} by first
>   extracting it into a variable with a fallback of '' and compare that
> * reorder the permission checks so that they are returned consistently
> * add patch that removes features when restoring an unprivileged
>   container as privileged
> 
> pve-container:
> 
> Dominik Csapak (3):
>   add old config and unprivileged to check_ct_modify_config_perm
>   allow nesting to be changed for VM.Allocate on unprivileged containers
>   skip features when restoring an unprivileged container as privileged
> 
>  src/PVE/API2/LXC.pm        |  6 +--
>  src/PVE/API2/LXC/Config.pm | 95 +++++++++++++++++++-------------------
>  src/PVE/LXC.pm             | 47 +++++++++++++++++--
>  src/PVE/LXC/Create.pm      |  5 ++
>  4 files changed, 100 insertions(+), 53 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