[PATCH-SERIES qemu-server/storage/common v2 0/4] fix UTF-8 handling for PBS_PASSWORD

[PATCH-SERIES qemu-server/storage/common v2 0/4] fix UTF-8 handling for PBS_PASSWORD

[Fiona Ebner]

Changes in v2:
* Improve commit message - the ENV issues only happen when Perl's
  internal representation is not valid UTF-8 itself.
* Rebase on latest master.

There are multiple issues with UTF-8 password handling for PBS backup:

1. As reported in the community forum, PBS backup of VMs would not
work when the password contained a multi-byte UTF-8 character. The
first patch fixes this by properly encoding the JSON sent via QMP.

2. The PBS password is saved as UTF-8 and decoded to Perl's internal
string representation upon reading from the password file. When the
password is not valid UTF-8 in Perl's internal representation, which
for example happens with a password like 'ääääöööö', backing up a
diskless VM would fail with:
> Error: error building client for repository XXX -
> PBS_PASSWORD contains bad characters

3. The same error would occur for uploading the log file after backup, as
well as extracting the configuration file from backup.

4. Restoring would fail with:
> restore failed: invalid utf-8 sequence of 1 bytes from index 0

Fix issues 2.-4. by properly encoding the value for the PBS_PASSWORD
environment variable value again as UTF-8.

5. For PMG, using passwords that would be auto-encoded by Perl as either
ASCII or UTF-8 already worked, but other encodings would not, for
example ISO-8859 would result in:
> proxmox-backup-client failed: Error: error building client for
> repository latin@pbs@10.10.100.180:8007:bigone - PBS_PASSWORD
> contains bad characters (500)

Follow pve-storage commit 5245e04 ("fix #5181: pbs: store and read
passwords as unicode") and align the behavior of the storage plugin
and pbs client module.

qemu-server:

Fiona Ebner (2):
  qmp client: encode JSON as UTF-8 to fix PBS backup when password
    contains multi-byte UTF-8
  pbs: properly encode PBS password as UTF-8 when setting the
    environment variable

 src/PVE/QMPClient.pm         | 14 ++++++++------
 src/PVE/QemuServer.pm        |  4 ++++
 src/PVE/VZDump/QemuServer.pm |  4 ++++
 3 files changed, 16 insertions(+), 6 deletions(-)


storage:

Fiona Ebner (1):
  pbs plugin: raw client command: properly encode PBS password as UTF-8
    when setting the environment variable

 src/PVE/Storage/PBSPlugin.pm | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)


common:

Fiona Ebner (1):
  pbs client: allow using password that would be auto-encoded as neither
    ASCII nor UTF-8

 src/PVE/PBSClient.pm | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)


Summary over all repositories:
  5 files changed, 32 insertions(+), 11 deletions(-)

-- 
Generated by git-murpp 0.5.0

[PATCH qemu-server v2 1/4] qmp client: encode JSON as UTF-8 to fix PBS backup when password contains multi-byte UTF-8

[Fiona Ebner]

As reported in the community forum, PBS backup of VMs would not work
when the password contained a multi-byte UTF-8 character [0].

The reason is that when writing a string in Perl's internal
representation with character values >= 128 to the QMP socket, QEMU
seems to have a bug where it reads too few characters (one less for
each char value >= 128). In such a case, the QMP client will just time
out, as the command is never completely read on the QEMU side.
Stuffing with additional characters would actually lead to QEMU
reading enough characters and executing the command, but Perl's
internal representation should not be submitted to QMP in the first
place. Encode the JSON properly as UTF-8 to avoid the issue.

In particular, this makes backing up a VM to PBS possible when the PBS
password contains multi-byte UTF-8 characters.

This also fixes future similar issues, for example when a QMP command
is passed a filesystem path with multi-byte UTF-8 characters.

[0]: https://forum.proxmox.com/threads/172871/post-804921

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

Unfortunately, doing the same for QGA is not yet enough to fix issue
#6609.

 src/PVE/QMPClient.pm | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/src/PVE/QMPClient.pm b/src/PVE/QMPClient.pm
index 723123a6..3cc5c3c8 100644
--- a/src/PVE/QMPClient.pm
+++ b/src/PVE/QMPClient.pm
@@ -295,12 +295,14 @@ my $check_queue = sub {
                     . "\n";
 
             } else {
-
-                $qmpcmd = to_json({
-                    execute => $cmd->{execute},
-                    arguments => $cmd->{arguments},
-                    id => $cmd->{id},
-                });
+                $qmpcmd = to_json(
+                    {
+                        execute => $cmd->{execute},
+                        arguments => $cmd->{arguments},
+                        id => $cmd->{id},
+                    },
+                    { utf8 => 1 },
+                );
             }
 
             if ($fd >= 0) {
-- 
2.47.3

[PATCH qemu-server v2 2/4] pbs: properly encode PBS password as UTF-8 when setting the environment variable

[Fiona Ebner]

The PBS password is saved as UTF-8 and decoded to Perl's internal
string representation upon reading from the password file. When the
password is not valid UTF-8 in Perl's internal representation, which
for example happens with a password like 'ääääöööö', backing up a
diskless VM would fail with:
> Error: error building client for repository XXX -
> PBS_PASSWORD contains bad characters
and restoring would fail with:
> restore failed: invalid utf-8 sequence of 1 bytes from index 0

This is fixed by properly encoding the value for the PBS_PASSWORD
environment variable value again as UTF-8.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

Changes in v2:
* Improve commit message - the ENV issues only happen when Perl's
  internal representation is not valid UTF-8 itself.

 src/PVE/QemuServer.pm        | 4 ++++
 src/PVE/VZDump/QemuServer.pm | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 38f34f80..dbc5e5ea 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -5,6 +5,7 @@ use warnings;
 
 use Cwd 'abs_path';
 use Digest::SHA;
+use Encode qw(encode);
 use Fcntl ':flock';
 use Fcntl;
 use File::Basename;
@@ -6880,6 +6881,9 @@ sub restore_proxmox_backup_archive {
 
     # This is only used for `pbs-restore` and the QEMU PBS driver (live-restore)
     my $password = PVE::Storage::PBSPlugin::pbs_get_password($scfg, $storeid);
+    # The password is saved as UTF-8 and is decoded upon reading. Need to re-encode when setting the
+    # environment variable.
+    $password = encode('UTF-8', $password, 1);
     local $ENV{PBS_PASSWORD} = $password;
     local $ENV{PBS_FINGERPRINT} = $fingerprint if defined($fingerprint);
 
diff --git a/src/PVE/VZDump/QemuServer.pm b/src/PVE/VZDump/QemuServer.pm
index 55fb6dc4..afac1496 100644
--- a/src/PVE/VZDump/QemuServer.pm
+++ b/src/PVE/VZDump/QemuServer.pm
@@ -3,6 +3,7 @@ package PVE::VZDump::QemuServer;
 use strict;
 use warnings;
 
+use Encode qw(encode);
 use Fcntl qw(:mode);
 use File::Basename;
 use File::Path qw(make_path remove_tree);
@@ -738,6 +739,9 @@ sub archive_pbs {
     if (!$diskcount) {
         $self->loginfo("backup contains no disks");
 
+        # The password is saved as UTF-8 and is decoded upon reading. Need to re-encode when setting
+        # the environment variable.
+        $password = encode('UTF-8', $password, 1);
         local $ENV{PBS_PASSWORD} = $password;
         local $ENV{PBS_FINGERPRINT} = $fingerprint if defined($fingerprint);
         my $cmd = [
-- 
2.47.3

[PATCH storage v2 3/4] pbs plugin: raw client command: properly encode PBS password as UTF-8 when setting the environment variable

[Fiona Ebner]

The PBS password is saved as UTF-8 and decoded to Perl's internal
string representation upon reading from the password file. When the
password is not valid UTF-8 in Perl's internal representation, which
for example happens with a password like 'ääääöööö', backing up a
diskless VM would fail with:
> Error: error building client for repository XXX -
> PBS_PASSWORD contains bad characters

This is fixed by properly encoding the value for the PBS_PASSWORD
environment variable value again as UTF-8.

For example, this fixes uploading the log file after backup, as well
as extracting the configuration file from backup.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

Changes in v2:
* Improve commit message - the ENV issues only happen when Perl's
  internal representation is not valid UTF-8 itself.

 src/PVE/Storage/PBSPlugin.pm | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/PVE/Storage/PBSPlugin.pm b/src/PVE/Storage/PBSPlugin.pm
index 17e285a..d81628d 100644
--- a/src/PVE/Storage/PBSPlugin.pm
+++ b/src/PVE/Storage/PBSPlugin.pm
@@ -5,7 +5,7 @@ package PVE::Storage::PBSPlugin;
 use strict;
 use warnings;
 
-use Encode qw(decode);
+use Encode qw(decode encode);
 use Fcntl qw(F_GETFD F_SETFD FD_CLOEXEC);
 use IO::File;
 use JSON;
@@ -327,7 +327,11 @@ my sub do_raw_client_cmd {
         push @$cmd, '--ns', $ns;
     }
 
-    local $ENV{PBS_PASSWORD} = pbs_get_password($scfg, $storeid);
+    my $password = pbs_get_password($scfg, $storeid);
+    # The password is saved as UTF-8 and is decoded upon reading. Need to re-encode when setting the
+    # environment variable.
+    $password = encode('UTF-8', $password, 1);
+    local $ENV{PBS_PASSWORD} = $password;
 
     local $ENV{PBS_FINGERPRINT} = $scfg->{fingerprint};
 
-- 
2.47.3

[PATCH common v2 4/4] pbs client: allow using password that would be auto-encoded as neither ASCII nor UTF-8

[Fiona Ebner]

Using passwords that would be auto-encoded by Perl as either ASCII or
UTF-8 already worked, but other encodings would not, for example
ISO-8859 would result in:
> proxmox-backup-client failed: Error: error building client for
> repository latin@pbs@10.10.100.180:8007:bigone - PBS_PASSWORD
> contains bad characters (500)

The issue only affected PMG, because in PVE, the PBS storage plugin
uses its own implementation of {get,set}_password() which does handle
UTF-8 already since pve-storage commit 5245e04 ("fix #5181: pbs: store
and read passwords as unicode"). Follow that commit to align the
behavior. This is also in preparation to using the PBS Client more
from the storage plugin too.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/PBSClient.pm | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
index 6333304..16d4740 100644
--- a/src/PVE/PBSClient.pm
+++ b/src/PVE/PBSClient.pm
@@ -4,6 +4,7 @@ package PVE::PBSClient;
 use strict;
 use warnings;
 
+use Encode qw(decode encode);
 use Fcntl qw(F_GETFD F_SETFD FD_CLOEXEC);
 use File::Temp qw(tempdir);
 use IO::File;
@@ -72,7 +73,7 @@ sub set_password {
     my $pwfile = password_file_name($self);
     mkdir($self->{secret_dir});
 
-    PVE::Tools::file_set_contents($pwfile, "$password\n", 0600);
+    PVE::Tools::file_set_contents($pwfile, "$password\n", 0600, 1);
 }
 
 sub delete_password {
@@ -88,7 +89,9 @@ sub get_password {
 
     my $pwfile = password_file_name($self);
 
-    return PVE::Tools::file_read_firstline($pwfile);
+    my $contents = PVE::Tools::file_read_firstline($pwfile);
+
+    return eval { decode('UTF-8', $contents, 1) } // $contents;
 }
 
 sub encryption_key_file_name {
@@ -185,7 +188,11 @@ my sub do_raw_client_cmd {
         push(@$cmd, '--ns', $ns);
     }
 
-    local $ENV{PBS_PASSWORD} = $self->get_password();
+    my $password = $self->get_password();
+    # The password is saved as UTF-8 and is decoded upon reading. Need to re-encode when setting the
+    # environment variable.
+    $password = encode('UTF-8', $password, 1);
+    local $ENV{PBS_PASSWORD} = $password;
 
     local $ENV{PBS_FINGERPRINT} = $scfg->{fingerprint};
 
-- 
2.47.3

Re: [PATCH-SERIES qemu-server/storage/common v2 0/4] fix UTF-8 handling for PBS_PASSWORD

[Maximiliano Sandoval]

Fiona Ebner <f.ebner@proxmox.com> writes:

> Changes in v2:
> * Improve commit message - the ENV issues only happen when Perl's
>   internal representation is not valid UTF-8 itself.
> * Rebase on latest master.
>
> There are multiple issues with UTF-8 password handling for PBS backup:
>
> 1. As reported in the community forum, PBS backup of VMs would not
> work when the password contained a multi-byte UTF-8 character. The
> first patch fixes this by properly encoding the JSON sent via QMP.
>
> 2. The PBS password is saved as UTF-8 and decoded to Perl's internal
> string representation upon reading from the password file. When the
> password is not valid UTF-8 in Perl's internal representation, which
> for example happens with a password like 'ääääöööö', backing up a
> diskless VM would fail with:
>> Error: error building client for repository XXX -
>> PBS_PASSWORD contains bad characters
>
> 3. The same error would occur for uploading the log file after backup, as
> well as extracting the configuration file from backup.
>
> 4. Restoring would fail with:
>> restore failed: invalid utf-8 sequence of 1 bytes from index 0
>
> Fix issues 2.-4. by properly encoding the value for the PBS_PASSWORD
> environment variable value again as UTF-8.
>
> 5. For PMG, using passwords that would be auto-encoded by Perl as either
> ASCII or UTF-8 already worked, but other encodings would not, for
> example ISO-8859 would result in:
>> proxmox-backup-client failed: Error: error building client for
>> repository latin@pbs@10.10.100.180:8007:bigone - PBS_PASSWORD
>> contains bad characters (500)
>
> Follow pve-storage commit 5245e04 ("fix #5181: pbs: store and read
> passwords as unicode") and align the behavior of the storage plugin
> and pbs client module.
>

I tested:

- Created a user on a pbs with bär as its password
- On a pve create a pbs storage for this user via pvesm add
- Check outs its `pvesm status`
- Created backups of a VM while if both offline and online
- The same but without any attached disk

It works as expected.

Tested-by: Maximiliano Sandoval <m.sandoval@proxmox.com>

-- 
Maximiliano