Re: [pve-devel] [PATCH qemu-server] config: add system and service credentials support

[Maximiliano Sandoval <m.sandoval@proxmox.com>]

Thomas Lamprecht <t.lamprecht@proxmox.com> writes:

> Am 02.04.25 um 16:36 schrieb Maximiliano Sandoval:
>> Allows to pass system and service credentials to a VM. See [1] for a
>> description of credentials. This can be potentially used to provision a
>> VM as per [2]. Values can be passed either as plain text or as a base64
>> encoded string when the base64 flag is set.
>
> Would this also make sense for Containers?

Absolutely, this would be a followup. The mechanism to pass them down to
containers is not via smbios 11 though.

> If it's something we can expose for all guests, we could also (later) look
> into implementing some simple registry (like a mappings type) fulfilling
> what the snippets approach would  provide one while having it nicely
> integrated into our access control system.

As per systemd-exec's man page, in total one can pass up to 1MB in
system credentials. A VM config file is certainly not the vehicle for
such an amount of data and I am also not fully comfortable with putting
potentially sensitive data as plain-text inside config files or the
cluster filesystem. I am not fully sure how to approach this long term.


There is also the more-secure possibility to pass down system
credentials from the host to the guest (e.g. ImportCredential= or
LoadCredential=) but that would have the drawback that there is no
mechanism to sync them acros a cluster.

>> 
>> A VM configuration file which, for example, contains:
>> 
>>     systemd-cred0: name=foo,value=bar
>>     systemd-cred1: name=encoded-foo,value=YmFy,base64=1
>
> Tangentially related: Moving the VM config fully over to section config
> parsing would get us list/array support for free – albeit the move might be
> rather costly..
>
>
>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index ffd5d56b..1f902b8b 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -150,6 +150,31 @@ my $watchdog_fmt = {
>>  };
>>  PVE::JSONSchema::register_format('pve-qm-watchdog', $watchdog_fmt);
>>  
>> +my $MAX_CREDENTIALS = 16;
>> +
>> +my $systemd_cred_fmt = {
>> +    value => {
>> +	description => 'The credential\'s value. This is a Base64 encoded string.'
>
> But it isn't if the base64 flag is not set?

I discussed this off-list a bit with Wolfgang and Fabian and I decided
for the time being to only accept the character set of base64, if a user
needs more control, they can encode the string as base64. But yes, the
description should be improved.

An alternative would be to require the string to be either a base64
string or match a hand-picked format, e.g. [a-zA-Z0-9\-\.], but that
would require studying what are the possible use-cases.

>> +	    .' If the value should contain arbitrary binary data,'
>
> nit: why the early line break in the mid of the same sentence?
>
>> +	    .' then the value can be a Base64 encoded string and the base64=1 flag should be set.',
>
> different casing: Base64 here and base64 below for the property
> with the same name.
>
>> +	type => 'string',
>> +	pattern => '[A-Za-z0-9+\/]+={0,2}',
>> +	typetext => '<string>',
>> +    },
>> +    name => {
>> +	description => 'The credential\'s name. This is a short ASCII string suitable as filename in the filesystem',
>> +	type => 'string',
>> +	pattern => '[A-Za-z0-9\-\._]+',
>
> The dot does not need to be escaped in a [characterset] pattern, and new use-sites
> might prefer using a quoted regex via qr/REGEX/ [perlop-regex-quote], which we
> already use on a few sites and has some small performance benefit and also
> features like having some regex-modifier flags enforced just for that sub
> pattern. I.e. the following should be the same: qr/[a-z0-9_.-]/i – not that
> you need to use exactly that here, just fyi in case you did not know about qr//.
>
> [perlop-regex-quote]: https://perldoc.perl.org/perlop#Regexp-Quote-Like-Operators
>
>> +	typetext => '<string>',
>> +    },
>> +    base64 => {
>> +	description => 'Whether the credential\'s value is base64 encoded.',
>> +	type => 'boolean',
>> +	optional => 1,
>> +	default => 0,
>> +    },
>> +};
>> +
>>  my $agent_fmt = {
>>      enabled => {
>>  	description => "Enable/disable communication with a QEMU Guest Agent (QGA) running in the VM.",
>> @@ -946,6 +971,13 @@ my $netdesc = {
>>      description => "Specify network devices.",
>>  };
>>  
>> +my $systemd_cred_desc = {
>> +    optional => 1,
>> +    type => 'string',
>> +    format => $systemd_cred_fmt,
>> +    description => 'Specify system and service credentials.',
>> +};
>> +
>>  PVE::JSONSchema::register_standard_option("pve-qm-net", $netdesc);
>>  
>>  my $ipconfig_fmt = {
>
>> @@ -1955,6 +1991,16 @@ sub parse_guest_agent {
>>      return $res;
>>  }
>>  
>> +sub parse_systemd_credential {
>> +    my ($value) = @_;
>> +
>> +    return {} if !$value;
>> +
>> +    my $res = eval { parse_property_string($systemd_cred_fmt, $value) };
>> +    warn $@ if $@;
> Should we really just ignore the error and warn here in a generic parser?
> Isn't that the callers job to decide?

Here I just copied what the other parsers in this file do. I am not sure
how to implement something else.

>
>> +    return $res;
>> +}
>> +
>>  sub get_qga_key {
>>      my ($conf, $key) = @_;
>>      return undef if !defined($conf->{agent});
>> @@ -3383,6 +3429,17 @@ my sub get_vga_properties {
>>      return ($vga, $qxlnum);
>>  }
>>  
>> +sub smbios_11_cred_arg {
>> +    my ($name, $value, $is_encoded) = @_;
>> +
>> +    if ($is_encoded) {
>> +	die "value $value is not base64 encoded\n" if $value !~ /^[A-Za-z0-9+\/]+={0,2}$/;
>
> A base64 regex might be good to have in a module wide variable, that then can
> also be used in the JSONSchema format above. Can be here in this module for now.
>
>> +	return ('-smbios', "type=11,value=io.systemd.credential.binary:$name=$value");
>> +    } else {
>> +	return ('-smbios', "type=11,value=io.systemd.credential:$name=$value");
>> +    }
>> +}
>> +
>>  sub config_to_command {
>>      my ($storecfg, $vmid, $conf, $defaults, $forcemachine, $forcecpu,
>>          $live_restore_backing) = @_;
>> @@ -4015,6 +4072,21 @@ sub config_to_command {
>>  	push @$cmd, '-snapshot';
>>      }
>>  
>> +    for (my $i = 0; $i < $MAX_CREDENTIALS; $i++)  {
>> +	my $cred_name = "systemd-cred$i";
>> +
>> +	next if !$conf->{$cred_name}
>
> might be tiny bit nicer if written as:
>
> my $cred = $conf->{"systemd-cred$i"} or next;
>
> ;
>> +
>> +	my $opts = parse_systemd_credential($conf->{$cred_name});
>> +	my $name = $opts->{'name'};
>> +	my $is_encoded = $opts->{'base64'};
>> +	my $value = $opts->{'value'};
>
> I'd avoid those useless intermediate variables, this part is not big enough
> to justify them IMO, just go for direct shell access.
>
>> +
>> +	if ($value && $name) {
>> +	    push @$cmd, smbios_11_cred_arg($name, $value, $is_encoded);
>> +	}
>> +    }
>> +
>>      # add custom args
>>      if ($conf->{args}) {
>>  	my $aa = PVE::Tools::split_args($conf->{args}); 



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