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

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

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?

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.

> 
> 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?

> +	    .' 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? 

> +    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