[pve-devel] [RFC container 3/3] implement permission checks for feature flags

[Thomas Lamprecht]

On 7/31/18 2:50 PM, Wolfgang Bumiller wrote:
> To disable a feature it is enough to be generally allowed
> to edit the configuration. Enabling a feature requires more
> privileges. For now: root at pam.
> 

While it is correct from a technical POV, it seems a bit strange from an
user experience POV, not sure about this.
E.g., I'm one of those people that often just try to toggle options for the
sake of it and see what happens - at least if it's nothing too important, 
and here I'd be quite bummed out if I had it, disabled keyctl and then my
unprivileged CT gets problems - no nice UX, IMO...

> Signed-off-by: Wolfgang Bumiller <w.bumiller at proxmox.com>
> ---
>  src/PVE/API2/LXC.pm        |  4 ++--
>  src/PVE/API2/LXC/Config.pm |  5 ++---
>  src/PVE/LXC.pm             | 42 +++++++++++++++++++++++++++++++++++++++++-
>  src/PVE/LXC/Config.pm      |  2 +-
>  4 files changed, 46 insertions(+), 7 deletions(-)
> 
> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> index 55e6310..dcdca3e 100644
> --- a/src/PVE/API2/LXC.pm
> +++ b/src/PVE/API2/LXC.pm
> @@ -244,7 +244,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, {}, $param, []);
>  
>  	my $storage_cfg = cfs_read_file("storage.cfg");
>  
> @@ -1567,7 +1567,7 @@ __PACKAGE__->register_method({
>  
>  	die "no options specified\n" if !scalar(keys %$param);
>  
> -	PVE::LXC::check_ct_modify_config_perm($rpcenv, $authuser, $vmid, undef, $param, []);
> +	PVE::LXC::check_ct_modify_config_perm($rpcenv, $authuser, $vmid, undef, {}, $param, []);
>  
>  	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 2b622b3..bfe6bb1 100644
> --- a/src/PVE/API2/LXC/Config.pm
> +++ b/src/PVE/API2/LXC/Config.pm
> @@ -112,7 +112,7 @@ __PACKAGE__->register_method({
>  	my $delete_str = extract_param($param, 'delete');
>  	my @delete = PVE::Tools::split_list($delete_str);
>  
> -	PVE::LXC::check_ct_modify_config_perm($rpcenv, $authuser, $vmid, undef, {}, [@delete]);
> +	PVE::LXC::check_ct_modify_config_perm($rpcenv, $authuser, $vmid, undef, {}, {}, [@delete]);
>  
>  	foreach my $opt (@delete) {
>  	    raise_param_exc({ delete => "you can't use '-$opt' and " .
> @@ -124,8 +124,6 @@ __PACKAGE__->register_method({
>  	    }
>  	}
>  
> -	PVE::LXC::check_ct_modify_config_perm($rpcenv, $authuser, $vmid, undef, $param, []);
> -
>  	my $storage_cfg = cfs_read_file("storage.cfg");
>  
>  	my $repl_conf = PVE::ReplicationConfig->new();
> @@ -156,6 +154,7 @@ __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, []);
>  
>  	    PVE::Tools::assert_if_modified($digest, $conf->{digest});
>  
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index 46222ba..d9ecbed 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -1026,8 +1026,41 @@ sub template_create {
>      PVE::LXC::Config->write_config($vmid, $conf);
>  }
>  
> +sub check_feature_modify_perm {
> +    my ($rpcenv, $authuser, $vmid, $pool, $old, $new) = @_;
> +
> +    my $features_desc = $PVE::LXC::Config::features_desc;
> +
> +    my $old_features = PVE::LXC::Config->parse_features($old);
> +    my $new_features = PVE::LXC::Config->parse_features($new);
> +
> +    foreach my $feature (keys %$new_features) {
> +	my $desc = $features_desc->{$feature};
> +	my $type = $desc->{type};
> +	my $new_value = $new_features->{$feature};
> +	# Explicitly disabling a feature is like removing a feature (Also see
> +	# the end of the function.)
> +	next if !defined($new_value) ||
> +		($type eq 'boolean' && !$new_value) ||
> +		!length($new_value);
> +
> +	my $old_value = $old_features->{$feature};
> +	# If we're not changing the feature's value, that's fine, too
> +	next if defined($old_value) && $new_value eq $old_value;
> +
> +	# Otherwise, we can have feature-specific checks here.
> +	# For now: require root at pam
> +	my $action = defined($old_value) ? 'modify' : 'enable';
> +	raise_perm_exc("only root\@pam can $action feature '$feature'")
> +	    if $authuser ne 'root at pam';
> +    }
> +
> +    # Whatever was in the old feature set abut not in the new feature set is
> +    # being removed, which requires no special permissions.
> +}
> +
>  sub check_ct_modify_config_perm {
> -    my ($rpcenv, $authuser, $vmid, $pool, $newconf, $delete) = @_;
> +    my ($rpcenv, $authuser, $vmid, $pool, $oldconf, $newconf, $delete) = @_;
>  
>      return 1 if $authuser eq 'root at pam';
>  
> @@ -1044,6 +1077,13 @@ sub check_ct_modify_config_perm {
>  		if $data->{type} ne 'volume';
>  	} elsif ($opt eq 'memory' || $opt eq 'swap') {
>  	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.Memory']);
> +	} elsif ($opt eq 'features') {
> +	    # In any case we need the usual VM.Config.Options
> +	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.Options']);
> +	    # But *adding* features requires special permissions:
> +	    if (!$delete) {
> +		check_feature_modify_perm($rpcenv, $authuser, $vmid, $pool, $oldconf->{$opt}, $newconf->{$opt});
> +	    }
>  	} elsif ($opt =~ m/^net\d+$/ || $opt eq 'nameserver' ||
>  		 $opt eq 'searchdomain' || $opt eq 'hostname') {
>  	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.Network']);
> diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
> index 9f6765e..61c3126 100644
> --- a/src/PVE/LXC/Config.pm
> +++ b/src/PVE/LXC/Config.pm
> @@ -272,7 +272,7 @@ PVE::JSONSchema::register_standard_option('pve-lxc-snapshot-name', {
>      maxLength => 40,
>  });
>  
> -my $features_desc = {
> +our $features_desc = {
>      mount => {
>  	optional => 1,
>  	type => 'string',
>