[pve-devel] [RFC PATCH 1/2] config: allow hypens and dots in config keys

[pve-devel] [RFC PATCH 1/2] config: allow hypens and dots in config keys

[Maximiliano Sandoval]

The first character still has to be a letter.

Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---
 PVE/QemuServer.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index b26da505..1566cf91 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -2382,7 +2382,7 @@ sub parse_vm_config {
 	    } else {
 		$handle_error->("vm $vmid - property 'delete' is only allowed in [PENDING]\n");
 	    }
-	} elsif ($line =~ m/^([a-z][a-z_]*\d*):\s*(.+?)\s*$/) {
+	} elsif ($line =~ m/^([a-z][a-z_\.-]*\d*):\s*(.+?)\s*$/) {
 	    my $key = $1;
 	    my $value = $2;
 	    if ($section eq 'cloudinit') {
-- 
2.39.5



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

[pve-devel] [RFC PATCH 2/2] config: add systemd credentials support

[Maximiliano Sandoval]

Allows to pass systemd credentials to a VM. See [1] for a description of
systemd credentials. This can be potentially used to provision a VM as
per [2]. Values can be passed either as plain text (which might be
base64 encrypted) or by reading the contents of a snippet.

A VM configuration file which, for example, contains:

    systemd.foo: value=bar
    systemd.ssh.authorized_keys.root: snippet=local:snippets/id_ed25519.pub
    systemd.encoded-foo: value=YmFya=,base64=1

will have the following arguments added to its kvm command:

    -smbios 'type=11,value=io.systemd.credential.binary:ssh.authorized_keys.root=c3NoLWVkMjU1MTkgQUFBQUMzTnphQzFsWkRJMU5URTVBQUFBSUZWZkFTYnVHdGdoWXBQQTBUS0w4N3I2dWRYNm5CbEM2L2hLWVZaTTdENzYgZm9vQGJhcgo=' \
    -smbios 'type=11,value=io.systemd.credential:foo=bar' \
    -smbios 'type=11,value=io.systemd.credential.binary:encoded-foo=YmFy'

On the guest these credentials can be read via:

    dmidecode -t 11

In the example above, the SSH key will be added to
/root/.ssh/authorized_keys provided the file did not exist, see [3].

[1] https://systemd.io/CREDENTIALS/
[2] https://www.freedesktop.org/software/systemd/man/latest/systemd.system-credentials.html
[3] https://www.freedesktop.org/software/systemd/man/latest/systemd.system-credentials.html#ssh.authorized_keys.root

Suggested-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---
 PVE/QemuServer.pm | 77 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 1566cf91..3ec21064 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -149,6 +149,26 @@ my $watchdog_fmt = {
 };
 PVE::JSONSchema::register_format('pve-qm-watchdog', $watchdog_fmt);
 
+our $systemd_value_fmt = {
+    value => {
+	description => 'The credential value. base64=1 should be specified if the value is base64 encoded.',
+	type => 'string',
+	optional => 1,
+    },
+    snippet => {
+	type => 'string',
+	description => "Specify a snippet containing the credential's value",
+	format => 'pve-volume-id',
+	optional => 1,
+    },
+    base64 => {
+	description => 'Whether the 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.",
@@ -2039,6 +2059,16 @@ sub parse_guest_agent {
     return $res;
 }
 
+sub parse_systemd_credential {
+    my ($value) = @_;
+
+    return {} if !$value;
+
+    my $res = eval { parse_property_string($systemd_value_fmt, $value) };
+    warn $@ if $@;
+    return $res;
+}
+
 sub get_qga_key {
     my ($conf, $key) = @_;
     return undef if !defined($conf->{agent});
@@ -2390,6 +2420,12 @@ sub parse_vm_config {
 		$conf->{$key} = $value;
 		next;
 	    }
+	    if ($key =~ /^systemd\.([a-z][a-z_\.-]*)$/) {
+		# ignore validation of systemd credentials
+		$conf->{'systemd-credentials'}->{$1} = $value;
+		next;
+	    }
+
 	    eval { $value = check_type($key, $value); };
 	    if ($@) {
 		$handle_error->("vm $vmid - unable to parse value of '$key' - $@");
@@ -3514,6 +3550,27 @@ my sub get_vga_properties {
     return ($vga, $qxlnum);
 }
 
+sub smbios_11_cred_arg {
+    my ($key, $value, $is_encoded) = @_;
+
+    if ($is_encoded) {
+	return ('-smbios', "type=11,value=io.systemd.credential.binary:$key=$value");
+    } else {
+	return ('-smbios', "type=11,value=io.systemd.credential:$key=$value");
+    }
+}
+
+sub read_systemd_custom_file {
+    my ($storage_conf, $path) = @_;
+
+    my ($vtype, undef) = PVE::Storage::parse_volname($storage_conf, $path);
+
+    die "$path is not in the snippets directory\n" if $vtype ne 'snippets';
+
+    my $full_path = PVE::Storage::abs_filesystem_path($storage_conf, $path, 1);
+    return PVE::Tools::file_get_contents($full_path, 1 * 1024 * 1024);
+}
+
 sub config_to_command {
     my ($storecfg, $vmid, $conf, $defaults, $forcemachine, $forcecpu,
         $live_restore_backing) = @_;
@@ -4142,6 +4199,26 @@ sub config_to_command {
 	push @$cmd, '-snapshot';
     }
 
+    # set systemd-credentials
+    my $storage_conf;
+    my $systemd_credentials = $conf->{'systemd-credentials'} || {};
+    foreach my $key (keys %$systemd_credentials) {
+	my $opts = parse_systemd_credential($systemd_credentials->{$key});
+	my $is_encoded = $opts->{'base64'} ? 1 : 0;
+	my $value;
+
+	if (my $v = $opts->{'value'}) {
+	    $value = $v;
+	} elsif (my $snippet = $opts->{'snippet'}) {
+	    $storage_conf = PVE::Storage::config() if !defined($storage_conf);
+	    my $contents = read_systemd_custom_file($storage_conf, $snippet);
+	    $value = encode_base64($contents, '');
+	    $is_encoded = 1;
+	}
+
+	push @$cmd, smbios_11_cred_arg($key, $value, $is_encoded) if $value;
+    }
+
     # add custom args
     if ($conf->{args}) {
 	my $aa = PVE::Tools::split_args($conf->{args});
-- 
2.39.5



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

Re: [pve-devel] [RFC PATCH 2/2] config: add systemd credentials support

[Fabian Grünbichler]

On September 24, 2024 4:35 pm, Maximiliano Sandoval wrote:
> Allows to pass systemd credentials to a VM. See [1] for a description of
> systemd credentials. This can be potentially used to provision a VM as
> per [2]. Values can be passed either as plain text (which might be
> base64 encrypted) or by reading the contents of a snippet.
> 
> A VM configuration file which, for example, contains:
> 
>     systemd.foo: value=bar
>     systemd.ssh.authorized_keys.root: snippet=local:snippets/id_ed25519.pub
>     systemd.encoded-foo: value=YmFya=,base64=1

why not our usual scheme?

systemd0: key=foo,value=bar
systemd1: key=ssh.authorized_keys.root,snippet=local:snippets/id_ed25519.pub
systemd2: key=encoded-foo,value=YmFya=,base64=1

if need be, the key could be persisted base64-encoded (e.g., if it can
contain characters used as delimiters or otherwise special)

> will have the following arguments added to its kvm command:
> 
>     -smbios 'type=11,value=io.systemd.credential.binary:ssh.authorized_keys.root=c3NoLWVkMjU1MTkgQUFBQUMzTnphQzFsWkRJMU5URTVBQUFBSUZWZkFTYnVHdGdoWXBQQTBUS0w4N3I2dWRYNm5CbEM2L2hLWVZaTTdENzYgZm9vQGJhcgo=' \
>     -smbios 'type=11,value=io.systemd.credential:foo=bar' \
>     -smbios 'type=11,value=io.systemd.credential.binary:encoded-foo=YmFy'
> 
> On the guest these credentials can be read via:
> 
>     dmidecode -t 11
> 
> In the example above, the SSH key will be added to
> /root/.ssh/authorized_keys provided the file did not exist, see [3].
> 
> [1] https://systemd.io/CREDENTIALS/
> [2] https://www.freedesktop.org/software/systemd/man/latest/systemd.system-credentials.html
> [3] https://www.freedesktop.org/software/systemd/man/latest/systemd.system-credentials.html#ssh.authorized_keys.root
> 
> Suggested-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
>  PVE/QemuServer.pm | 77 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 77 insertions(+)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 1566cf91..3ec21064 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -149,6 +149,26 @@ my $watchdog_fmt = {
>  };
>  PVE::JSONSchema::register_format('pve-qm-watchdog', $watchdog_fmt);
>  
> +our $systemd_value_fmt = {
> +    value => {
> +	description => 'The credential value. base64=1 should be specified if the value is base64 encoded.',
> +	type => 'string',

nack - this needs some sort of restriction (e.g. a sane character set
for non-base64 encoded values, or base64)

> +	optional => 1,
> +    },
> +    snippet => {
> +	type => 'string',
> +	description => "Specify a snippet containing the credential's value",
> +	format => 'pve-volume-id',

not too sure about adding yet more usage to the already overloaded and
problematic snippet feature that we want to get rid of/overhaul..

> +	optional => 1,
> +    },
> +    base64 => {
> +	description => 'Whether the 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.",
> @@ -2039,6 +2059,16 @@ sub parse_guest_agent {
>      return $res;
>  }
>  
> +sub parse_systemd_credential {
> +    my ($value) = @_;
> +
> +    return {} if !$value;
> +
> +    my $res = eval { parse_property_string($systemd_value_fmt, $value) };
> +    warn $@ if $@;

why? if you do this, it needs to be optin via a $noerr parameter..

> +    return $res;
> +}
> +
>  sub get_qga_key {
>      my ($conf, $key) = @_;
>      return undef if !defined($conf->{agent});
> @@ -2390,6 +2420,12 @@ sub parse_vm_config {

you only touch parse_vm_config.. how does this work on read-modify-write
cycles??

>  		$conf->{$key} = $value;
>  		next;
>  	    }
> +	    if ($key =~ /^systemd\.([a-z][a-z_\.-]*)$/) {
> +		# ignore validation of systemd credentials
> +		$conf->{'systemd-credentials'}->{$1} = $value;
> +		next;
> +	    }
> +

this part here would not be needed if it were a regular config option
instead of getting special treatment..

>  	    eval { $value = check_type($key, $value); };

but this part here would still trigger schema validation then, which
would be a plus!

>  	    if ($@) {
>  		$handle_error->("vm $vmid - unable to parse value of '$key' - $@");
> @@ -3514,6 +3550,27 @@ my sub get_vga_properties {
>      return ($vga, $qxlnum);
>  }
>  
> +sub smbios_11_cred_arg {
> +    my ($key, $value, $is_encoded) = @_;
> +
> +    if ($is_encoded) {
> +	return ('-smbios', "type=11,value=io.systemd.credential.binary:$key=$value");
> +    } else {
> +	return ('-smbios', "type=11,value=io.systemd.credential:$key=$value");
> +    }
> +}
> +
> +sub read_systemd_custom_file {
> +    my ($storage_conf, $path) = @_;
> +
> +    my ($vtype, undef) = PVE::Storage::parse_volname($storage_conf, $path);
> +
> +    die "$path is not in the snippets directory\n" if $vtype ne 'snippets';
> +
> +    my $full_path = PVE::Storage::abs_filesystem_path($storage_conf, $path, 1);
> +    return PVE::Tools::file_get_contents($full_path, 1 * 1024 * 1024);
> +}

this is a 1:1 copy of
PVE::QemuServer::Cloudinit::read_cloudinit_snippets_file ..

> +
>  sub config_to_command {
>      my ($storecfg, $vmid, $conf, $defaults, $forcemachine, $forcecpu,
>          $live_restore_backing) = @_;
> @@ -4142,6 +4199,26 @@ sub config_to_command {
>  	push @$cmd, '-snapshot';
>      }
>  
> +    # set systemd-credentials
> +    my $storage_conf;
> +    my $systemd_credentials = $conf->{'systemd-credentials'} || {};
> +    foreach my $key (keys %$systemd_credentials) {
> +	my $opts = parse_systemd_credential($systemd_credentials->{$key});
> +	my $is_encoded = $opts->{'base64'} ? 1 : 0;
> +	my $value;
> +
> +	if (my $v = $opts->{'value'}) {
> +	    $value = $v;
> +	} elsif (my $snippet = $opts->{'snippet'}) {
> +	    $storage_conf = PVE::Storage::config() if !defined($storage_conf);

why? config_to_command already has a copy of the storage config..

> +	    my $contents = read_systemd_custom_file($storage_conf, $snippet);
> +	    $value = encode_base64($contents, '');
> +	    $is_encoded = 1;
> +	}
> +
> +	push @$cmd, smbios_11_cred_arg($key, $value, $is_encoded) if $value;
> +    }
> +
>      # add custom args
>      if ($conf->{args}) {
>  	my $aa = PVE::Tools::split_args($conf->{args});
> -- 
> 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

Re: [pve-devel] [RFC PATCH 1/2] config: allow hypens and dots in config keys

[Fiona Ebner]

Am 24.09.24 um 16:35 schrieb Maximiliano Sandoval:
> The first character still has to be a letter.
> 
> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
>  PVE/QemuServer.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index b26da505..1566cf91 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -2382,7 +2382,7 @@ sub parse_vm_config {
>  	    } else {
>  		$handle_error->("vm $vmid - property 'delete' is only allowed in [PENDING]\n");
>  	    }
> -	} elsif ($line =~ m/^([a-z][a-z_]*\d*):\s*(.+?)\s*$/) {
> +	} elsif ($line =~ m/^([a-z][a-z_\.-]*\d*):\s*(.+?)\s*$/) {
>  	    my $key = $1;
>  	    my $value = $2;
>  	    if ($section eq 'cloudinit') {

Regarding the hyphen, see also:
https://lore.proxmox.com/pve-devel/7c911e11-d1cb-415b-9282-b40faa34dc9b@proxmox.com/#t

Also friendly ping for @Thomas for that mail, in particular, for where
this should be documented.


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

Re: [pve-devel] [RFC PATCH 1/2] config: allow hypens and dots in config keys

[Thomas Lamprecht]

s/hypens/hyphens/

Am 24/09/2024 um 16:35 schrieb Maximiliano Sandoval:
> The first character still has to be a letter.

Such a patch really must have more rationale for why each character
is required, e.g. for hyphens it would be allowing using kebab-case
like we use for (most) new schema properties, might be obvious to
you, but it hasn't to be obvious for every dev or (potential)
contributor. Dot on the other hand isnt' obvious to me, do we use
that anywhere, why should it be allowed for properties if we want
to switch everything to kebab-case in the  long term? As this would
only allow adding further inconsistency?


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