Re: [pve-devel] [PATCH qemu-server v3 10/11] allow non-root users to set /dev/u?random as an RNG source

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On February 10, 2025 4:37 pm, Filip Schauer wrote:
> Allow non-root users with the VM.Config.HWType privilege to configure
> /dev/urandom & /dev/random as an entropy source for a VirtIO RNG device.
> /dev/hwrng remains restricted to the root user.
> 
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
> ---
>  PVE/API2/Qemu.pm  | 43 +++++++++++++++++++++++++++++++++++++++++++
>  PVE/QemuServer.pm | 13 +++++++++++--
>  2 files changed, 54 insertions(+), 2 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 295260e7..194e6357 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -38,6 +38,7 @@ use PVE::QemuServer::Memory qw(get_current_memory);
>  use PVE::QemuServer::MetaInfo;
>  use PVE::QemuServer::PCI;
>  use PVE::QemuServer::QMPHelpers;
> +use PVE::QemuServer::RNG;
>  use PVE::QemuServer::USB;
>  use PVE::QemuMigrate;
>  use PVE::RPCEnvironment;
> @@ -673,6 +674,7 @@ my $hwtypeoptions = {
>      'vga' => 1,
>      'watchdog' => 1,
>      'audio0' => 1,
> +    'rng0' => 1,
>  };
>  
>  my $generaloptions = {
> @@ -801,6 +803,36 @@ my sub check_vm_create_hostpci_perm {
>      return 1;
>  };
>  
> +my sub check_rng_perm {
> +    my ($rpcenv, $authuser, $vmid, $pool, $opt, $value) = @_;
> +
> +    return 1 if $authuser eq 'root@pam';
> +
> +    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.HWType']);
> +
> +    my $device = PVE::JSONSchema::parse_property_string('pve-qm-rng', $value);
> +    if ($device->{source}) {
> +	if ($device->{source} eq '/dev/hwrng') {
> +	    die "only root can set '$opt' config for a non-mapped Hardware RNG device\n";
> +	}
> +    }
> +
> +    return 1;
> +}
> +
> +my sub check_vm_create_rng_perm {
> +    my ($rpcenv, $authuser, $vmid, $pool, $param) = @_;
> +
> +    return 1 if $authuser eq 'root@pam';
> +
> +    foreach my $opt (keys %{$param}) {
> +	next if $opt !~ m/^rng\d+$/;
> +	check_rng_perm($rpcenv, $authuser, $vmid, $pool, $opt, $param->{$opt});
> +    }
> +
> +    return 1;
> +};

there only ever is one rng device at the moment, so this could be
dropped..

> +
>  my $check_vm_modify_config_perm = sub {
>      my ($rpcenv, $authuser, $vmid, $pool, $key_list) = @_;
>  
> @@ -1114,6 +1146,7 @@ __PACKAGE__->register_method({
>  	    &$check_vm_create_serial_perm($rpcenv, $authuser, $vmid, $pool, $param);
>  	    check_vm_create_usb_perm($rpcenv, $authuser, $vmid, $pool, $param);
>  	    check_vm_create_hostpci_perm($rpcenv, $authuser, $vmid, $pool, $param);
> +	    check_vm_create_rng_perm($rpcenv, $authuser, $vmid, $pool, $param);

and this could instead be

check_rng_perm($rpcenv, $authuser, $vmid, $pool, 'rng0', $param->{rng0})
    if $param->{rng0};

?

>  
>  	    PVE::QemuServer::check_bridge_access($rpcenv, $authuser, $param);
>  	    &$check_cpu_model_access($rpcenv, $authuser, $param);
> @@ -2005,6 +2038,10 @@ my $update_vm_api  = sub {
>  		    check_hostpci_perm($rpcenv, $authuser, $vmid, undef, $opt, $val);
>  		    PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
>  		    PVE::QemuConfig->write_config($vmid, $conf);
> +		} elsif ($opt eq m/^rng\d+$/) {
> +		    check_rng_perm($rpcenv, $authuser, $vmid, undef, $opt, $val);
> +		    PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
> +		    PVE::QemuConfig->write_config($vmid, $conf);
>  		} elsif ($opt eq 'tags') {
>  		    assert_tag_permissions($vmid, $val, '', $rpcenv, $authuser);
>  		    delete $conf->{$opt};
> @@ -2095,6 +2132,12 @@ my $update_vm_api  = sub {
>  		    }
>  		    check_hostpci_perm($rpcenv, $authuser, $vmid, undef, $opt, $param->{$opt});
>  		    $conf->{pending}->{$opt} = $param->{$opt};
> +		} elsif ($opt =~ m/^rng\d+$/) {
> +		    if (my $oldvalue = $conf->{$opt}) {
> +			check_rng_perm($rpcenv, $authuser, $vmid, undef, $opt, $oldvalue);
> +		    }
> +		    check_rng_perm($rpcenv, $authuser, $vmid, undef, $opt, $param->{$opt});
> +		    $conf->{pending}->{$opt} = $param->{$opt};
>  		} elsif ($opt eq 'tags') {
>  		    assert_tag_permissions($vmid, $conf->{$opt}, $param->{$opt}, $rpcenv, $authuser);
>  		    $conf->{pending}->{$opt} = PVE::GuestHelpers::get_unique_tags($param->{$opt});
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 70518924..cc69eeb1 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -6398,8 +6398,17 @@ sub check_mapping_access {
>  	    } else {
>  		die "either 'host' or 'mapping' must be set.\n";
>  	    }
> -       }
> -   }
> +	} elsif ($opt =~ m/^rng\d+$/) {
> +	    my $device = PVE::JSONSchema::parse_property_string('pve-qm-rng', $conf->{$opt});
> +
> +	    if ($device->{source}) {
> +		if ($device->{source} eq '/dev/hwrng') {
> +		    die "only root can set '$opt' config for a non-mapped Hardware RNG device\n"
> +			if $user ne 'root@pam';
> +		}
> +	    }
> +	}

those are three nested ifs that could be a single one?

if ($device->{source} && $device->{source} eq '/dev/hwrng' && $user ne 'root@pam') {

}

I guess we could even move the root check for the whole sub up front as
a precursor patch, to simplify the if conditions a bit..

I also wonder whether it makes sense to allow /dev/urandom and
/dev/random as mappings if they are not restricted in the first place
when used directly? what's the advantage of that?

> +    }
>  };
>  
>  sub check_restore_permissions {
> -- 
> 2.39.5
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel