Thomas Lamprecht <t.lampre...@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
