[PATCH common,qemu-server,network,manager 0/10] mark secrets in schemas and redact them from the system report

[PATCH common,qemu-server,network,manager 0/10] mark secrets in schemas and redact them from the system report

[Thomas Lamprecht]

The system report only requires Sys.Audit, but contains all guest configs
and the SDN configs verbatim. So it includes cloud-init passwords, in plain
text for Windows guests, and the API tokens of SDN IPAM and DNS
integrations. A report is often sent to support by someone who does not
own these credentials, for example a hosting provider renting out VMs.

This series adds a 'sensitive' flag for schema properties and uses it to
redact the report:

 - pve-common gets the flag, and helpers that replace the values of
   flagged properties in the raw text of key/value and section config
   files. Working on the raw text keeps invalid lines visible, which are
   often what support needs to see.
 - qemu-server flags the cloud-init password, and no longer shows it in
   the warning about an unparsable config line. pve-network flags the
   IPAM and DNS tokens.
 - The report redacts guest, storage and SDN IPAM/DNS configs through
   their schemas. In the whole report it also replaces passwords in URLs
   and values whose name suggests a credential, like mount options and
   container environment variables. The pvereport man page lists what
   gets redacted and where credentials can remain.

The flag only declares a value as secret; it changes neither validation
nor how values are stored. It can later also drive generic masking of API
output and task logs.

If it looks alright in general I can then add support for this also on
the rust side, partially prepared here already locally. And we can then
also leverage this more, like midterm also do a slightly more elaborate
secret-store concept with the biggest benefit being an unified API and
potentially different backends (pmxcfs, simple file store) and (at rest)
encryption and so on.

Repo (build-)dependencies:
 qemu-server, pve-network, pve-manager -> pve-common
The report change only has an effect with the qemu-server and pve-network
patches applied.

pve-common:

Thomas Lamprecht (4):
  section config: do not add isolated property lists on lookups
  schema: allow marking properties as sensitive
  schema: add helpers to redact sensitive values
  section config: add redaction of sensitive values in raw configs

 src/PVE/JSONSchema.pm                         |  51 ++++++
 src/PVE/SectionConfig.pm                      |  51 +++++-
 test/json-schema-test.pl                      | 148 ++++++++++++++++++
 .../section_config_property_isolation_test.pl |  52 ++++++
 test/section_config_test.pl                   |  40 +++++
 5 files changed, 341 insertions(+), 1 deletion(-)

qemu-server:

Thomas Lamprecht (2):
  cloud-init: mark the password as sensitive
  config: hide sensitive values of unparsable lines in warnings

 src/PVE/QemuServer.pm | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

pve-network:

Thomas Lamprecht (1):
  sdn: ipam, dns: mark API tokens as sensitive

 src/PVE/Network/SDN/Dns/PowerdnsPlugin.pm  | 1 +
 src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm | 1 +
 2 files changed, 2 insertions(+)

pve-manager:

Thomas Lamprecht (3):
  report: redact sensitive values from guest, storage and SDN configs
  report: redact credentials in URLs, options and environment variables
  pvereport: man page: document which values get redacted

 PVE/Report.pm       | 78 ++++++++++++++++++++++++++++++++++++++++-----
 bin/pvereport       | 31 ++++++++++++++++++
 test/Makefile       |  6 +++-
 test/report_test.pl | 78 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 184 insertions(+), 9 deletions(-)
 create mode 100755 test/report_test.pl

-- 
2.47.3

[PATCH common 01/10] section config: do not add isolated property lists on lookups

[Thomas Lamprecht]

With isolated properties in use, looking up a property of a type that
has no isolated property list, like a plugin that is not isolated or an
unknown type, created an empty list for that type as a side effect.
Only init() reads these lists, and it starts from scratch, so nothing
depended on the extra entries. Still, a lookup should not modify the
plugin data, especially when the type comes from a config file.

No semantic change intended.

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
 src/PVE/SectionConfig.pm                       | 3 ++-
 test/section_config_property_isolation_test.pl | 9 +++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm
index 66375ac..1febf76 100644
--- a/src/PVE/SectionConfig.pm
+++ b/src/PVE/SectionConfig.pm
@@ -1306,7 +1306,8 @@ sub get_property_schema {
 
     my $schema;
     if ($class->has_isolated_properties()) {
-        $schema = $pdata->{isolatedPropertyList}->{$type}->{$key};
+        my $isolated = $pdata->{isolatedPropertyList}->{$type};
+        $schema = $isolated->{$key} if $isolated;
     }
     $schema //= $pdata->{propertyList}->{$key};
 
diff --git a/test/section_config_property_isolation_test.pl b/test/section_config_property_isolation_test.pl
index 4c999c1..11b3142 100755
--- a/test/section_config_property_isolation_test.pl
+++ b/test/section_config_property_isolation_test.pl
@@ -472,6 +472,15 @@ my $update_schema = Conf->updateSchema();
 my $expected_update_schema = make_schema(1);
 is_deeply($update_schema, $expected_update_schema, "property-isolation update schema test");
 
+ok(
+    !defined(Conf->get_property_schema('unknown', 'another')),
+    "unknown type has no isolated properties",
+);
+ok(
+    !exists(Conf->private()->{isolatedPropertyList}->{unknown}),
+    "property lookup does not add a property list",
+);
+
 done_testing();
 
 1;
-- 
2.47.3

[PATCH common 02/10] schema: allow marking properties as sensitive

[Thomas Lamprecht]

Credentials such as cloud-init passwords and SDN API tokens are stored
inline in configs, and from there end up in places with a wider
audience, like the system report, which only requires Sys.Audit. Code
that needs to treat such values specially currently has to know their
property names, as the cipassword masking in qemu-server does.

The new flag lets the schema declare a value as a secret, so generic
code can recognize it. It does not affect validation or how a value is
stored. It can be set on any property schema, including the keys of
property-string formats.

The meta schema is only checked by the verifyapi commands, so older
versions of this package do not break at runtime when they see the
flag.

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
 src/PVE/JSONSchema.pm | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
index 34f2949..e4225df 100644
--- a/src/PVE/JSONSchema.pm
+++ b/src/PVE/JSONSchema.pm
@@ -1791,6 +1791,14 @@ my $default_schema_noref = {
             optional => 1,
             default => 0,
         },
+        sensitive => {
+            type => "boolean",
+            description =>
+                "Whether the value is a secret or derived from one, like a password, a"
+                . " password hash or an API token.",
+            optional => 1,
+            default => 0,
+        },
         properties => {
             type => "object",
             description => "This is a definition for the properties of an object value",
-- 
2.47.3

[PATCH common 03/10] schema: add helpers to redact sensitive values

[Thomas Lamprecht]

The system report includes config files verbatim. Redacting them by
parsing and writing them out again would drop or normalize invalid
lines, which are often what support needs to see. So redaction works on
the raw text of key/value config files and only replaces the values of
properties marked as sensitive, in every section of the file. For
property strings only the sensitive keys are replaced, the rest of the
value stays readable. Empty values are kept, as they reveal nothing and
help to spot configuration errors.

The value helper also takes parsed values, including array references
and unset values, so it can be reused for masking API output.

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
 src/PVE/JSONSchema.pm    |  43 ++++++++++++
 test/json-schema-test.pl | 148 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 191 insertions(+)

diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
index e4225df..12ef6c2 100644
--- a/src/PVE/JSONSchema.pm
+++ b/src/PVE/JSONSchema.pm
@@ -2656,6 +2656,49 @@ sub dump_config {
     return $data;
 }
 
+sub redact_sensitive_value($schema, $value, $placeholder) {
+    return $value if !defined($value);
+    return [map { redact_sensitive_value($schema, $_, $placeholder) } $value->@*]
+        if ref($value) eq 'ARRAY';
+    return $value if $value !~ m/\S/;
+    return $placeholder if $schema->{sensitive};
+    return redact_sensitive_value($schema->{items}, $value, $placeholder) if $schema->{items};
+
+    my $format = $schema->{format};
+    $format = get_format($format) if defined($format) && !ref($format);
+    return $value if ref($format) ne 'HASH';
+
+    my ($default_key) = grep { $format->{$_}->{default_key} } keys $format->%*;
+    my $key_schema = sub($key) {
+        my $key_schema = $format->{$key} // return;
+        return defined($key_schema->{alias}) ? $format->{ $key_schema->{alias} } : $key_schema;
+    };
+
+    my @parts = split(/,/, $value, -1);
+    for my $part (@parts) {
+        if (my ($key, $key_value) = $part =~ m/^([^=]+)=(.*)$/) {
+            my $schema = $key_schema->($key) or next;
+            $part = "$key=" . redact_sensitive_value($schema, $key_value, $placeholder);
+        } elsif (defined($default_key)) {
+            $part = redact_sensitive_value($format->{$default_key}, $part, $placeholder);
+        }
+    }
+    return join(',', @parts);
+}
+
+sub redact_raw_config($schema, $raw, $placeholder) {
+    my $props = $schema->{properties} or die "got strange schema\n";
+
+    my @lines = split(/\n/, $raw, -1);
+    for my $line (@lines) {
+        my ($prefix, $key, $value) = $line =~ m/^(\s*(\S+?):\s*)(.*)$/ or next;
+        my $prop = $props->{$key} or next;
+        $prop = $props->{ $prop->{alias} } // next if defined($prop->{alias});
+        $line = $prefix . redact_sensitive_value($prop, $value, $placeholder);
+    }
+    return join("\n", @lines);
+}
+
 # helpers used to generate our manual pages
 
 my $find_schema_default_key = sub {
diff --git a/test/json-schema-test.pl b/test/json-schema-test.pl
index f1c968f..28c8efd 100755
--- a/test/json-schema-test.pl
+++ b/test/json-schema-test.pl
@@ -863,4 +863,152 @@ for my $test ($check_one_of->@*) {
     };
 }
 
+PVE::JSONSchema::register_format(
+    'test-redact',
+    {
+        name => { type => 'string', default_key => 1, optional => 1 },
+        secret => { type => 'string', sensitive => 1, optional => 1 },
+        'old-secret' => { alias => 'secret' },
+    },
+);
+
+my $redact_value_tests = [
+    {
+        name => 'sensitive value',
+        schema => { type => 'string', sensitive => 1 },
+        in => 'hunter2',
+        out => '***',
+    },
+    {
+        name => 'other value',
+        schema => { type => 'string' },
+        in => 'visible',
+        out => 'visible',
+    },
+    {
+        name => 'sensitive sub-key',
+        schema => { type => 'string', format => 'test-redact' },
+        in => 'name=foo,secret=bar',
+        out => 'name=foo,secret=***',
+    },
+    {
+        name => 'sensitive sub-key via alias',
+        schema => { type => 'string', format => 'test-redact' },
+        in => 'foo,old-secret=bar',
+        out => 'foo,old-secret=***',
+    },
+    {
+        name => 'sensitive default key',
+        schema => {
+            type => 'string',
+            format => { secret => { type => 'string', default_key => 1, sensitive => 1 } },
+        },
+        in => 'bar,,',
+        out => '***,,',
+    },
+    {
+        name => 'format without sensitive sub-keys',
+        schema => { type => 'string', format => { name => { type => 'string' } } },
+        in => 'not=a,valid',
+        out => 'not=a,valid',
+    },
+    {
+        name => 'empty sensitive value',
+        schema => { type => 'string', sensitive => 1 },
+        in => '',
+        out => '',
+    },
+    {
+        name => 'nested format',
+        schema => {
+            type => 'string',
+            format => {
+                inner => { type => 'string', format => 'test-redact' },
+                other => { type => 'string' },
+            },
+        },
+        in => 'other=x,inner=secret=bar',
+        out => 'other=x,inner=secret=***',
+    },
+    {
+        name => 'unset value',
+        schema => { type => 'string', sensitive => 1 },
+        in => undef,
+        out => undef,
+    },
+    {
+        name => 'array item',
+        schema => { type => 'array', items => { type => 'string', format => 'test-redact' } },
+        in => 'foo,secret=bar',
+        out => 'foo,secret=***',
+    },
+    {
+        name => 'array of items',
+        schema => { type => 'array', items => { type => 'string', format => 'test-redact' } },
+        in => ['foo,secret=bar', 'baz'],
+        out => ['foo,secret=***', 'baz'],
+    },
+    {
+        name => 'sensitive array',
+        schema => { type => 'array', sensitive => 1, items => { type => 'string' } },
+        in => ['foo', 'bar'],
+        out => ['***', '***'],
+    },
+];
+
+for my $test ($redact_value_tests->@*) {
+    my $res = PVE::JSONSchema::redact_sensitive_value($test->{schema}, $test->{in}, '***');
+    is_deeply($res, $test->{out}, "redact $test->{name}");
+}
+
+my $redact_config_schema = {
+    type => 'object',
+    properties => {
+        password => { type => 'string', sensitive => 1 },
+        'old-password' => { type => 'string', alias => 'password' },
+        options => { type => 'string', format => 'test-redact' },
+        name => { type => 'string' },
+        tokens => { type => 'array', items => { type => 'string', sensitive => 1 } },
+    },
+};
+my $redact_config_in = <<'EOF';
+#password: in a comment
+password: hunter2
+old-password: aliased
+options: foo,secret=bar
+name: visible
+tokens: first
+tokens: second
+invalid line
+
+[snapshot]
+  password:   older
+[special:cloudinit]
+password: applied
+password:
+EOF
+my $redact_config_out = <<'EOF';
+#password: in a comment
+password: ***
+old-password: ***
+options: foo,secret=***
+name: visible
+tokens: ***
+tokens: ***
+invalid line
+
+[snapshot]
+  password:   ***
+[special:cloudinit]
+password: ***
+password:
+EOF
+is(
+    PVE::JSONSchema::redact_raw_config($redact_config_schema, $redact_config_in, '***'),
+    $redact_config_out,
+    'redact key/value config',
+);
+eval { PVE::JSONSchema::redact_raw_config({ type => 'object' }, $redact_config_in, '***') };
+like($@, qr/strange schema/, 'redact key/value config fails without properties');
+
 done_testing();
-- 
2.47.3

[PATCH common 04/10] section config: add redaction of sensitive values in raw configs

[Thomas Lamprecht]

Section config files like the SDN IPAM and DNS configs hold API tokens
and are included verbatim in the system report, so they need the same
raw-text redaction as key/value config files.

Sections are delimited as in parse_config, so each value is checked
against the schema of the section the parser assigns it to. Sections of
a type without a registered plugin are still checked against the
properties that are not isolated to a plugin.

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
 src/PVE/SectionConfig.pm                      | 48 +++++++++++++++++++
 .../section_config_property_isolation_test.pl | 43 +++++++++++++++++
 test/section_config_test.pl                   | 40 ++++++++++++++++
 3 files changed, 131 insertions(+)

diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm
index 1febf76..04a79de 100644
--- a/src/PVE/SectionConfig.pm
+++ b/src/PVE/SectionConfig.pm
@@ -1807,6 +1807,54 @@ sub write_config {
     return $out;
 }
 
+=head3 $base->redact_raw_config(...)
+
+=head3 $base->redact_raw_config($raw, $placeholder)
+
+    $redacted = PVE::Example::BasePlugin->redact_raw_config($raw, $placeholder)
+    $redacted = $class->redact_raw_config($raw, $placeholder)
+
+Returns the contents of a C<L<PVE::SectionConfig>> file with the value of every
+property marked as C<sensitive> replaced by C<$placeholder>, including sensitive
+keys of property strings.
+
+Works on the raw text, so lines that
+C<L<< parse_config()|/$base->parse_config(...) >>> would skip as invalid are
+kept. As in C<L<< parse_config()|/$base->parse_config(...) >>>, a section ends
+at the next empty line. Outside of sections of a known
+I<L<< type|/$plugin->type() >>>, only properties that are not isolated to a
+plugin are recognized.
+
+=cut
+
+sub redact_raw_config($class, $raw, $placeholder) {
+    my ($type, $in_section) = ('', 0);
+    my @lines = split(/\n/, $raw, -1);
+    for my $line (@lines) {
+        next if $line =~ m/^\s*#/;
+
+        if ($line eq '') {
+            ($type, $in_section) = ('', 0);
+            next;
+        }
+
+        if (!$in_section) {
+            my ($header_type) = $class->parse_section_header($line);
+            if (defined($header_type)) {
+                ($type, $in_section) = ($header_type, 1);
+                next;
+            }
+        }
+
+        if (my ($prefix, $key, $value) = $line =~ m/^(\s+(\S+)\s+)(.*)$/) {
+            my $schema = $class->get_property_schema($type, $key) or next;
+            $line =
+                $prefix . PVE::JSONSchema::redact_sensitive_value($schema, $value, $placeholder);
+        }
+    }
+    return join("\n", @lines);
+}
+
 sub assert_if_modified {
     my ($cfg, $digest) = @_;
 
diff --git a/test/section_config_property_isolation_test.pl b/test/section_config_property_isolation_test.pl
index 11b3142..a487952 100755
--- a/test/section_config_property_isolation_test.pl
+++ b/test/section_config_property_isolation_test.pl
@@ -85,6 +85,7 @@ sub properties {
             description => 'Another field',
             type => 'string',
             optional => 1,
+            sensitive => 1,
         },
         arrayfield => {
             description => "Array Field with property string",
@@ -395,6 +396,7 @@ my sub make_schema($is_update) {
                                 description => 'Another field',
                                 type => 'string',
                                 optional => 1,
+                                sensitive => 1,
                             },
                             arrayfield => {
                                 description => "Array Field with property string",
@@ -481,6 +483,47 @@ ok(
     "property lookup does not add a property list",
 );
 
+my $redact_in = <<"EOF";
+one: a
+	another secret
+
+two: b
+	another visible
+
+unknown: c
+	another isolated
+
+two: d
+one: d-ignored
+	another visible
+
+one: e
+two: e-ignored
+	another secret
+	another
+EOF
+my $redact_out = <<"EOF";
+one: a
+	another <redacted>
+
+two: b
+	another visible
+
+unknown: c
+	another isolated
+
+two: d
+one: d-ignored
+	another visible
+
+one: e
+two: e-ignored
+	another <redacted>
+	another
+EOF
+is(Conf->redact_raw_config($redact_in, '<redacted>'), $redact_out,
+    "property-isolation redact test");
+
 done_testing();
 
 1;
diff --git a/test/section_config_test.pl b/test/section_config_test.pl
index 2cdff98..3e52834 100755
--- a/test/section_config_test.pl
+++ b/test/section_config_test.pl
@@ -75,6 +75,7 @@ sub properties {
         another => {
             description => 'Another field',
             type => 'string',
+            sensitive => 1,
         },
     };
 }
@@ -115,6 +116,7 @@ sub properties {
                     subfield1 => {
                         type => 'string',
                         description => 'first subfield',
+                        sensitive => 1,
                     },
                     subfield2 => {
                         type => 'integer',
@@ -300,6 +302,7 @@ my $expected_create_schema = {
                     'subfield1' => {
                         'description' => 'first subfield',
                         'type' => 'string',
+                        'sensitive' => 1,
                     },
                 },
                 'type' => 'string',
@@ -311,6 +314,7 @@ my $expected_create_schema = {
             'description' => 'Another field',
             'optional' => 1,
             'type' => 'string',
+            'sensitive' => 1,
         },
     },
 };
@@ -369,6 +373,7 @@ my $expected_update_schema = {
                     subfield1 => {
                         description => 'first subfield',
                         type => 'string',
+                        sensitive => 1,
                     },
                 },
             },
@@ -379,11 +384,46 @@ my $expected_update_schema = {
             description => 'Another field',
             optional => 1,
             type => 'string',
+            sensitive => 1,
         },
     },
 };
 is_deeply($update_schema, $expected_update_schema, "update schema test");
 
+my $redact_in = <<"EOF";
+one: t1
+	common foo
+	another secret1
+
+# another: comment
+two: t2
+	field2 5
+	another  secret2
+invalid line
+	arrayfield subfield2=2,subfield1=secret3
+
+bad: unknown
+	another secret4
+	unknownfield visible
+EOF
+my $redact_out = <<"EOF";
+one: t1
+	common foo
+	another <redacted>
+
+# another: comment
+two: t2
+	field2 5
+	another  <redacted>
+invalid line
+	arrayfield subfield2=2,subfield1=<redacted>
+
+bad: unknown
+	another <redacted>
+	unknownfield visible
+EOF
+is(Conf->redact_raw_config($redact_in, '<redacted>'), $redact_out, "redact raw config");
+
 done_testing();
 
 1;
-- 
2.47.3

[PATCH qemu-server 05/10] cloud-init: mark the password as sensitive

[Thomas Lamprecht]

For Windows guests the password is stored in plain text, as
cloudbase-init needs it that way, and for other guests it is a crypt
hash, which still allows offline guessing. Marking it lets generic code
like the system report redact it without knowing the property name.

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
NOTE: 'qm verifyapi' runs in the docs build and fails with a
libpve-common-perl that does not know the 'sensitive' schema flag yet,
so the build dependency needs a bump once that is released.

 src/PVE/QemuServer.pm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index b4a1a38d..2a3e266b 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -800,6 +800,7 @@ my $confdesc_cloudinit = {
     cipassword => {
         optional => 1,
         type => 'string',
+        sensitive => 1,
         description => 'cloud-init: Password to assign the user. Using this is generally not'
             . ' recommended. Use ssh keys instead. Also note that older cloud-init versions do not'
             . ' support hashed passwords.',
-- 
2.47.3

[PATCH qemu-server 06/10] config: hide sensitive values of unparsable lines in warnings

[Thomas Lamprecht]

A config line that does not parse is echoed verbatim in the warning,
which ends up in task logs, the journal and the system report. For a
password line that is only broken by its formatting, for example by
leading whitespace, the warning disclosed the password, even though the
report redacts it in the config itself.

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
NOTE: needs a libpve-common-perl with the raw config redaction helpers,
as a hard runtime dependency.

 src/PVE/QemuServer.pm | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 2a3e266b..aa1e2e9b 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -2036,7 +2036,10 @@ sub parse_vm_config {
                 $conf->{$key} = $value;
             }
         } else {
-            $handle_error->("vm $vmid - unable to parse config: $line\n");
+            my $shown = PVE::JSONSchema::redact_raw_config(
+                { properties => $confdesc }, $line, '<hidden>',
+            );
+            $handle_error->("vm $vmid - unable to parse config: $shown\n");
         }
     }
 
-- 
2.47.3

[PATCH network 07/10] sdn: ipam, dns: mark API tokens as sensitive

[Thomas Lamprecht]

The phpIPAM and Netbox tokens and the PowerDNS API key are stored in
plain text in the cluster-wide IPAM and DNS configs. Marking them lets
generic code like the system report redact them.

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
NOTE: needs a libpve-common-perl that knows the 'sensitive' schema
flag, otherwise 'pvesh verifyapi' rejects the IPAM and DNS API schemas.

 src/PVE/Network/SDN/Dns/PowerdnsPlugin.pm  | 1 +
 src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm | 1 +
 2 files changed, 2 insertions(+)

diff --git a/src/PVE/Network/SDN/Dns/PowerdnsPlugin.pm b/src/PVE/Network/SDN/Dns/PowerdnsPlugin.pm
index dfb7932..3d88ea4 100644
--- a/src/PVE/Network/SDN/Dns/PowerdnsPlugin.pm
+++ b/src/PVE/Network/SDN/Dns/PowerdnsPlugin.pm
@@ -24,6 +24,7 @@ sub properties {
         },
         key => {
             type => 'string',
+            sensitive => 1,
         },
         reversemaskv6 => {
             type => 'integer',
diff --git a/src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm b/src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm
index 395d72c..99378a4 100644
--- a/src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm
+++ b/src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm
@@ -19,6 +19,7 @@ sub properties {
         },
         token => {
             type => 'string',
+            sensitive => 1,
         },
         section => {
             type => 'integer',
-- 
2.47.3

[PATCH manager 08/10] report: redact sensitive values from guest, storage and SDN configs

[Thomas Lamprecht]

The system report only requires Sys.Audit, but includes all guest
configs and the SDN configs verbatim. So it contains cloud-init
passwords, in plain text for Windows guests, and the API tokens of SDN
IPAM and DNS integrations, while the config API masks the password for
everyone. A report is often sent to support by someone who does not own
these credentials, for example a hosting provider renting out VMs.

Replace the values of all properties marked as sensitive in their
schema, in every section including snapshots and pending changes. The
key stays visible, and so does every other line of the file, including
invalid ones.

storage.cfg holds no secrets of the built-in plugins, which store them
in separate files, but third-party plugins that keep credentials inline
can now mark them and have them redacted too.

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
NOTE: needs a libpve-common-perl with the raw config redaction helpers
as a hard runtime dependency; with an older one the report shows errors
instead of the guest, storage and SDN configs. The redaction only takes
effect with qemu-server and libpve-network-perl versions that mark their
secrets.

 PVE/Report.pm | 56 ++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 49 insertions(+), 7 deletions(-)

diff --git a/PVE/Report.pm b/PVE/Report.pm
index e255d538f..7182c091d 100644
--- a/PVE/Report.pm
+++ b/PVE/Report.pm
@@ -4,11 +4,27 @@ use strict;
 use warnings;
 
 use PVE::IPRoute2;
+use PVE::JSONSchema;
+use PVE::LXC::Config;
+use PVE::Network::SDN::Dns;
+use PVE::Network::SDN::Ipams;
+use PVE::QemuServer;
+use PVE::Storage;
 use PVE::Tools;
 
+my $redaction_placeholder = '<redacted>';
+
+my sub file2text {
+    my ($path, $redact) = @_;
+
+    my $content = PVE::Tools::file_get_contents($path);
+    $content = $redact->($content, $path) if $redact;
+    return "\n# cat $path\n$content\n";
+}
+
 # output the content of all the files of a directory
 my sub dir2text {
-    my ($target_dir, $regexp) = @_;
+    my ($target_dir, $regexp, $redact) = @_;
 
     print STDERR "dir2text '${target_dir}${regexp}'...";
     my $text = "# output '${target_dir}${regexp}' file(s)\n";
@@ -18,13 +34,39 @@ my sub dir2text {
         sub {
             my ($file) = @_;
             return if $file eq '.' || $file eq '..';
-            $text .= "\n# cat $target_dir$file\n";
-            $text .= PVE::Tools::file_get_contents($target_dir . $file) . "\n";
+            $text .= file2text($target_dir . $file, $redact);
         },
     );
     return $text;
 }
 
+my sub redact_qemu_config {
+    my ($raw) = @_;
+
+    my $schema = { type => 'object', properties => PVE::QemuServer::json_config_properties({}) };
+    return PVE::JSONSchema::redact_raw_config($schema, $raw, $redaction_placeholder);
+}
+
+my sub redact_lxc_config {
+    my ($raw) = @_;
+
+    my $schema = { type => 'object', properties => PVE::LXC::Config->json_config_properties({}) };
+    return PVE::JSONSchema::redact_raw_config($schema, $raw, $redaction_placeholder);
+}
+
+my $section_configs = {
+    '/etc/pve/sdn/dns.cfg' => 'PVE::Network::SDN::Dns::Plugin',
+    '/etc/pve/sdn/ipams.cfg' => 'PVE::Network::SDN::Ipams::Plugin',
+    '/etc/pve/storage.cfg' => 'PVE::Storage::Plugin',
+};
+
+my sub redact_section_config {
+    my ($raw, $path) = @_;
+
+    my $class = $section_configs->{$path} // return $raw;
+    return $class->redact_raw_config($raw, $redaction_placeholder);
+}
+
 # command -v is the posix equivalent of 'which'
 my sub cmd_exists { system("command -v '$_[0]' > /dev/null 2>&1") == 0 }
 
@@ -59,7 +101,7 @@ my $init_report_cmds = sub {
         storage => {
             order => 30,
             cmds => [
-                'cat /etc/pve/storage.cfg',
+                sub { file2text('/etc/pve/storage.cfg', \&redact_section_config) },
                 'pvesm status',
                 'cat /etc/fstab',
                 'findmnt --ascii',
@@ -71,9 +113,9 @@ my $init_report_cmds = sub {
             order => 40,
             cmds => [
                 'qm list',
-                sub { dir2text('/etc/pve/qemu-server/', '\d+\.conf') },
+                sub { dir2text('/etc/pve/qemu-server/', '\d+\.conf', \&redact_qemu_config) },
                 'pct list',
-                sub { dir2text('/etc/pve/lxc/', '\d+\.conf') },
+                sub { dir2text('/etc/pve/lxc/', '\d+\.conf', \&redact_lxc_config) },
             ],
         },
         network => {
@@ -85,7 +127,7 @@ my $init_report_cmds = sub {
                 'cat /etc/network/interfaces',
                 sub { dir2text('/etc/network/interfaces.d/', '.*') },
                 'cat /etc/pve/sdn/.running-config',
-                sub { dir2text('/etc/pve/sdn/', '.+\.cfg') },
+                sub { dir2text('/etc/pve/sdn/', '.+\.cfg', \&redact_section_config) },
                 sub { dir2text('/etc/pve/sdn/', '.+\.json') },
             ],
         },
-- 
2.47.3

[PATCH manager 09/10] report: redact credentials in URLs, options and environment variables

[Thomas Lamprecht]

Some credentials in the report are not covered by any schema: the
password in a URL, as used in APT sources or the HTTP proxy setting of
datacenter.cfg, mount options like password= in fstab, and container
environment variables like DB_PASSWORD, which the env property stores as
raw lxc keys. Replace passwords in URLs and values whose name suggests a
credential, in the whole report.

This is a heuristic. It also catches such options on command lines as
shown by top, but not values passed as separate arguments, nor the part
of a value after a space or comma, except for environment variables.

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
 PVE/Report.pm       | 22 ++++++++++++-
 test/Makefile       |  6 +++-
 test/report_test.pl | 78 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 104 insertions(+), 2 deletions(-)
 create mode 100755 test/report_test.pl

diff --git a/PVE/Report.pm b/PVE/Report.pm
index 7182c091d..fa7fc4280 100644
--- a/PVE/Report.pm
+++ b/PVE/Report.pm
@@ -67,6 +67,26 @@ my sub redact_section_config {
     return $class->redact_raw_config($raw, $redaction_placeholder);
 }
 
+# names like password, db_pass, ADMIN_TOKEN, SECRET_KEY or api_key
+my $credential_name_re = qr/
+    (?<![a-z0-9])
+    (?: (?:[a-z0-9]+_)* (?:password2?|passwd|pass|secret|token) (?:_[a-z0-9]+)* | (?:[a-z0-9]+_)+ key )
+/ix;
+
+# credentials in URLs, like in APT sources or proxy settings, in container environment variables
+# and in other name=value pairs, like mount options
+sub redact_credentials {
+    my ($text) = @_;
+
+    $text =~
+        s{(?<![a-z0-9+.-])([a-z][a-z0-9+.-]*+://[^\s/:]*):[^\s/]+@}{$1:$redaction_placeholder\@}gi;
+    $text =~
+        s/^(lxc\.environment(?:\.\w+)?(?::|\s*=)\s*$credential_name_re=).+$/$1$redaction_placeholder/gm;
+    $text =~ s/($credential_name_re=)(?:,,|[^\s,])+/$1$redaction_placeholder/g;
+
+    return $text;
+}
+
 # command -v is the posix equivalent of 'which'
 my sub cmd_exists { system("command -v '$_[0]' > /dev/null 2>&1") == 0 }
 
@@ -276,7 +296,7 @@ sub generate {
         }
     }
 
-    return $report;
+    return redact_credentials($report);
 }
 
 1;
diff --git a/test/Makefile b/test/Makefile
index 2dfd20a41..91761579d 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -5,7 +5,7 @@ all:
 export PERLLIB=..
 
 .PHONY: check
-check: test-replication test-balloon test-vzdump test-osd test-ceph-health test-ceph-services test-ceph-auth test-ceph-key-migration test-ceph-lockbox-migration test-custom-cpu-models test-pvesh
+check: test-replication test-balloon test-vzdump test-osd test-ceph-health test-ceph-services test-ceph-auth test-ceph-key-migration test-ceph-lockbox-migration test-custom-cpu-models test-pvesh test-report
 
 .PHONY: test-balloon
 test-balloon:
@@ -61,6 +61,10 @@ test-custom-cpu-models:
 test-pvesh:
 	./pvesh_test.pl
 
+.PHONY: test-report
+test-report:
+	./report_test.pl
+
 .PHONY: install
 install:
 
diff --git a/test/report_test.pl b/test/report_test.pl
new file mode 100755
index 000000000..444e4344f
--- /dev/null
+++ b/test/report_test.pl
@@ -0,0 +1,78 @@
+#!/usr/bin/perl
+
+use v5.36;
+
+use lib ('.', '..');
+
+use Test::More;
+
+use PVE::Report;
+
+my $tests = [
+    [
+        'APT source with credentials',
+        'deb https://user:s3cr:et@deb.example.com/debian trixie main',
+        'deb https://user:<redacted>@deb.example.com/debian trixie main',
+    ],
+    [
+        'URL with an unencoded @ in the password',
+        'http_proxy: http://user:p@ss@proxy.example.com:3128/',
+        'http_proxy: http://user:<redacted>@proxy.example.com:3128/',
+    ],
+    [
+        'URL with an @ in the user name',
+        'http_proxy: http://user@corp.example:s3cret@proxy.example.com:3128/',
+        'http_proxy: http://user@corp.example:<redacted>@proxy.example.com:3128/',
+    ],
+    [
+        'proxy URL with port and without credentials',
+        'http_proxy: http://proxy.example.com:3128/',
+        'http_proxy: http://proxy.example.com:3128/',
+    ],
+    [
+        'URL with user only',
+        'URIs: https://user@deb.example.com/debian',
+        'URIs: https://user@deb.example.com/debian',
+    ],
+    [
+        'URL with user and port only',
+        'ssh://root@host.example:22/srv',
+        'ssh://root@host.example:22/srv',
+    ],
+    [
+        'CIFS mount options',
+        '//srv/share /mnt cifs username=foo,password=bar,uid=0 0 0',
+        '//srv/share /mnt cifs username=foo,password=<redacted>,uid=0 0 0',
+    ],
+    [
+        'CIFS password with an escaped comma',
+        'username=foo,password=a,,b,password2=new,uid=0',
+        'username=foo,password=<redacted>,password2=<redacted>,uid=0',
+    ],
+    [
+        'Ceph mount options',
+        'mon1:/ /mnt ceph name=admin,secret=AQB+abc==,secretfile=/etc/ceph/s 0 0',
+        'mon1:/ /mnt ceph name=admin,secret=<redacted>,secretfile=/etc/ceph/s 0 0',
+    ],
+    [
+        'command line options',
+        'tool --pass=hunter2 --bypass=1 --db-password=x --api_key=y passthrough=on',
+        'tool --pass=<redacted> --bypass=1 --db-password=<redacted> --api_key=<redacted> passthrough=on',
+    ],
+    [
+        'container environment',
+        "lxc.environment.runtime: ADMIN_TOKEN=\$argon2id\$v=19\$m=65540,t=3,p=4\$c2FsdA\$aGFzaA\n"
+            . "lxc.environment.runtime: DB_PASSWORD=correct horse\n"
+            . "lxc.environment.runtime: TZ=Europe/Vienna\n",
+        "lxc.environment.runtime: ADMIN_TOKEN=<redacted>\n"
+            . "lxc.environment.runtime: DB_PASSWORD=<redacted>\n"
+            . "lxc.environment.runtime: TZ=Europe/Vienna\n",
+    ],
+];
+
+for my $test ($tests->@*) {
+    my ($name, $in, $out) = $test->@*;
+    is(PVE::Report::redact_credentials($in), $out, $name);
+}
+
+done_testing();
-- 
2.47.3

[PATCH manager 10/10] pvereport: man page: document which values get redacted

[Thomas Lamprecht]

Admins who send a report to support need to know which secrets it
leaves out, and which possibly sensitive information it still contains.

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
 bin/pvereport | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/bin/pvereport b/bin/pvereport
index 8658a0424..fb9320a98 100755
--- a/bin/pvereport
+++ b/bin/pvereport
@@ -22,3 +22,34 @@ pvereport
 =head1 DESCRIPTION
 
 Display various information related to a Proxmox VE system
+
+The report replaces the following values with C<< <redacted> >>:
+
+=over
+
+=item *
+
+Values of options marked as secret in guest, storage, SDN IPAM and SDN DNS
+configurations, like the cloud-init password of guests and the API tokens of
+IPAM and DNS plugins.
+
+=item *
+
+Passwords in URLs, for example in APT sources or the HTTP proxy setting.
+
+=item *
+
+Values in C<name=value> form whose name suggests a credential, like
+C<password> in mount options or C<DB_PASSWORD> in the environment of a
+container.
+
+=back
+
+This only covers the cases above. Credentials can remain elsewhere, for example
+in command lines, in QEMU C<args>, in the output of C<ceph config dump>, in
+error messages, or in options of storage plugins that are not installed on this
+node. The report also contains other information that can be sensitive, like
+guest descriptions, user names, SSH public keys, IP addresses and host names.
+Check the report before you share it.
+
+The same applies to the system report generated in the web interface.
-- 
2.47.3