From 49149bb922e016d22578e56b584194ff349020e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vin=C3=ADcius=20Ferr=C3=A3o?= <2031761+viniciusferrao@users.noreply.github.com> Date: Sun, 9 Aug 2026 14:01:11 -0300 Subject: [PATCH 1/3] fix(xcatd): redact node password attributes from logged commands redact_password only knew about bmcdiscover, mkhwconn and rspconfig, so setting a password the ordinary way left it in the clear: [Request] chdef node01 'bmcpassword=SEKRET' Those secrets are carried as an assignment on whichever command happens to set them, so match them by name rather than by command. The names are every attribute Schema.pm maps to a password, passwd, authkey or privkey column, and the columns themselves, since a table qualified assignment such as passwd.password= is accepted too. An assignment may be written with spaces around the equals sign and the value may contain spaces, so a quoted argument is redacted to its closing quote. Attributes such as key, which names a monitoring attribute, and sshkeydir, which is a directory, are not secrets and are left readable. The existing per-command table is kept for a flag like bmcdiscover -p, which carries its secret in the following argument where there is no name to match. --- xCAT-server/lib/perl/xCAT/xcatd.pm | 44 ++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/xCAT-server/lib/perl/xCAT/xcatd.pm b/xCAT-server/lib/perl/xCAT/xcatd.pm index bfe235174..c4755136a 100644 --- a/xCAT-server/lib/perl/xCAT/xcatd.pm +++ b/xCAT-server/lib/perl/xCAT/xcatd.pm @@ -562,6 +562,50 @@ sub redact_password { } } } + # Object definition attributes carry their value as attr=value, on whichever + # command happens to set them, so they are matched by name rather than by + # command. These are the attributes that map to a secret column in + # Schema.pm; attributes such as 'key' and 'sshkeydir' are not secrets and + # are deliberately absent. + my @password_attributes = qw( + authkey + bmcpassword + domainadminpassword + iscsipassword + passwd.HMC + passwd.admin + passwd.celogin + passwd.general + passwd.hscroot + password + privkey + snmppassword + + domain.adminpassword + ipmi.password + iscsi.passwd + mpa.password + openbmc.password + passwd.password + pdu.authkey + pdu.password + pdu.privkey + ppcdirect.password + ppchcp.password + switches.password + switches.sshpassword + vm.vidpassword + websrv.password + ); + foreach my $attribute (@password_attributes) { + # An assignment may be written with spaces around the equals sign and + # the value may itself contain spaces, in which case the argument + # arrives quoted. Redact to the closing quote there, and to the end of + # the argument otherwise, so the rest of the command stays readable. + $parameters =~ s/(^|\s)'(\Q$attribute\E\s*=\s*)[^']*'/$1'$2$redact_string'/g; + $parameters =~ s/(^|\s)(\Q$attribute\E\s*=\s*)[^'\s]*/$1$2$redact_string/g; + } + # Return original request with password replaced by 'x' in $parameters string if ($request =~ '\[Request\]') { return $header . "[Request] " . $command . " " . $parameters; From 65f7b212659c1b42b14d84573546ce44b5ce9e4f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vin=C3=ADcius=20Ferr=C3=A3o?= <2031761+viniciusferrao@users.noreply.github.com> Date: Sun, 9 Aug 2026 14:01:11 -0300 Subject: [PATCH 2/3] fix(xcatd): redact the arguments stored in the auditlog table Only syslog received the redacted arguments. The auditlog table was given the raw string, so a password removed from syslog and from commands.log was still written to the database, which persists and is readable by anything with access to it. Use the redacted text for both. This also covers the mkvm --password and -w masking a few lines above, which the table did not have either. --- xCAT-server/lib/perl/xCAT/xcatd.pm | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/xCAT-server/lib/perl/xCAT/xcatd.pm b/xCAT-server/lib/perl/xCAT/xcatd.pm index c4755136a..3bafeeb83 100644 --- a/xCAT-server/lib/perl/xCAT/xcatd.pm +++ b/xCAT-server/lib/perl/xCAT/xcatd.pm @@ -271,8 +271,14 @@ sub validate { $saveArglist = "$first$restcommand"; } } - # Replace passwords with 'x' - if ($arglist) { $logst .= redact_password($request->{command}->[0], $saveArglist); } + # Replace passwords with 'x'. The same redacted text is used for + # syslog and for the auditlog table, which recorded the raw + # arguments and so kept secrets that syslog did not. + my $redacted_arglist = $arglist; + if ($arglist) { + $redacted_arglist = redact_password($request->{command}->[0], $saveArglist); + $logst .= $redacted_arglist; + } if ($peername) { $logst .= " for " . $request->{username}->[0] } if ($peerhost) { $logst .= " from " . $peerhost } @@ -336,7 +342,7 @@ sub validate { if ($request->{noderange} && defined($request->{noderange}->[0])) { $rsp->{noderange}->[0] = $request->{noderange}->[0]; } - $rsp->{args}->[0] = $arglist; + $rsp->{args}->[0] = $redacted_arglist; $rsp->{status}->[0] = $status; if ($skipsyslog == 0) { # write to syslog and auditlog @$deferredmsgargs = ("SA", $rsp); From fca07665fe616731e0a9e8f9e6ab16ae2a20da8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vin=C3=ADcius=20Ferr=C3=A3o?= <2031761+viniciusferrao@users.noreply.github.com> Date: Sun, 9 Aug 2026 14:01:11 -0300 Subject: [PATCH 3/3] test(xcatd): cover password redaction in the logged commands Run redact_password rather than inspecting its source, so that a secret which survives is a failure here rather than something the shape of the code hides. Each supported way of writing an assignment is exercised, including the table qualified form and spaces around the equals sign, together with the positional flag that the per-command table handles. The expected set is derived from Schema.pm rather than repeated, so an attribute added there with a secret column fails this test instead of quietly reaching the logs, and the test bails if that mapping cannot be parsed so it cannot pass vacuously. Detail that is not secret is asserted to survive, since redacting it would cost the log its usefulness without protecting anything. --- xCAT-test/unit/xcatd_password_redaction.t | 87 +++++++++++++++++++++++ 1 file changed, 87 insertions(+) create mode 100644 xCAT-test/unit/xcatd_password_redaction.t diff --git a/xCAT-test/unit/xcatd_password_redaction.t b/xCAT-test/unit/xcatd_password_redaction.t new file mode 100644 index 000000000..bf45db751 --- /dev/null +++ b/xCAT-test/unit/xcatd_password_redaction.t @@ -0,0 +1,87 @@ +#!/usr/bin/env perl +use strict; +use warnings; + +use FindBin; +use File::Spec; +use Test::More; + +my $repo_root = File::Spec->catdir( $FindBin::Bin, '..', '..' ); +my $xcatd = File::Spec->catfile( $repo_root, 'xCAT-server/lib/perl/xCAT/xcatd.pm' ); +my $schema = File::Spec->catfile( $repo_root, 'perl-xCAT/xCAT/Schema.pm' ); + +plan skip_all => 'xcatd.pm or Schema.pm not found' unless -r $xcatd && -r $schema; + +sub slurp { + open( my $fh, '<', $_[0] ) or die "Unable to read $_[0]: $!"; + my $c = do { local $/; <$fh> }; + close($fh); + return $c; +} + +my $source = slurp($xcatd); +my $schema_source = slurp($schema); + +# Run the real routine rather than inspecting its source. xcatd.pm cannot be +# loaded here because of its dependencies, so the routine is lifted out and +# evaluated on its own; it uses nothing outside itself. +my ($routine) = $source =~ /(sub redact_password \{.*?\n\}\n)/s; +ok( $routine, 'redact_password was located in xcatd.pm' ) + or BAIL_OUT('xcatd.pm no longer defines redact_password'); +eval "package RedactUnderTest; $routine 1;" or BAIL_OUT("could not evaluate redact_password: $@"); + +sub redacted { return RedactUnderTest::redact_password( $_[0], $_[1] ); } + +# A secret must never survive, whichever way it was written. +my %leaks = ( + 'attribute assignment' => [ 'chdef', " node01 bmcpassword=SEKRET" ], + 'quoted assignment' => [ 'chdef', " node01 'bmcpassword=SEKRET'" ], + 'spaces around the equals' => [ 'chdef', " node01 'bmcpassword = SEKRET phrase'" ], + 'table qualified column' => [ 'chtab', " key=system passwd.username=root passwd.password=SEKRET" ], + 'several on one command' => [ 'nodeadd', " n1 ipmi.password=SEKRET openbmc.password=SEKRET" ], + 'snmp passphrases' => [ 'chdef', " pdu1 authkey=SEKRET privkey=SEKRET" ], + 'positional flag' => [ 'bmcdiscover', " -s nmap -p SEKRET --range 10.0.0.1" ], +); +foreach my $case ( sort keys %leaks ) { + my ( $command, $args ) = @{ $leaks{$case} }; + unlike( redacted( $command, $args ), qr/SEKRET/, "no secret survives: $case" ); +} + +# Detail that is not secret has to stay, or the log stops being useful. +my $kept = redacted( 'chdef', " node01 sshkeydir=/etc/xcat/keys key=system groups=lab mgt=ipmi" ); +like( $kept, qr/sshkeydir=\/etc\/xcat\/keys/, 'sshkeydir is kept, it is a directory' ); +like( $kept, qr/key=system/, 'key is kept, it names a monitoring attribute' ); +like( $kept, qr/groups=lab/, 'unrelated attributes are kept' ); +like( + redacted( 'chtab', " key=system passwd.username=root passwd.password=SEKRET" ), + qr/passwd\.username=root/, + 'the username beside a redacted password is kept' +); + +# Every attribute Schema.pm maps to a secret column has to be covered, so that +# one added later fails here instead of quietly reaching the logs. +my %expected; +while ( $schema_source =~ /attr_name\s*=>\s*'([^']+)'(.{0,400}?)tabentry\s*=>\s*'([^']+)'/gs ) { + my ( $attr, $tabentry ) = ( $1, $3 ); + next unless $tabentry =~ /\.(password|passwd|authkey|privkey|adminpassword|sshpassword)$/i; + $expected{$attr} = $tabentry; +} +ok( scalar( keys %expected ) > 0, 'Schema.pm yielded attributes mapped to secret columns' ) + or BAIL_OUT('the Schema.pm mapping could not be parsed, so this test proves nothing'); + +my @uncovered; +foreach my $attr ( sort keys %expected ) { + push @uncovered, $attr + if redacted( 'chdef', " node01 $attr=SEKRET" ) =~ /SEKRET/; + my $column = $expected{$attr}; + push @uncovered, $column + if redacted( 'chdef', " node01 $column=SEKRET" ) =~ /SEKRET/; +} +is_deeply( \@uncovered, [], 'every attribute and column Schema.pm marks secret is redacted' ); + +# The auditlog table was given the raw arguments while syslog was given the +# redacted ones, so both sinks must now use the same text. +like( $source, qr/\$redacted_arglist\s*=\s*redact_password/, 'the redacted arguments are computed once' ); +like( $source, qr/\$rsp->\{args\}->\[0\]\s*=\s*\$redacted_arglist/, 'the auditlog table stores the redacted arguments' ); + +done_testing();