From 031ad68a41477c3f67a199fa12d878de72eb9795 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vin=C3=ADcius=20Ferr=C3=A3o?= <2031761+viniciusferrao@users.noreply.github.com> Date: Thu, 20 Aug 2026 19:37:56 -0300 Subject: [PATCH 1/2] fix(xcatd): classify secret responses by the shared secret set The commands.log response classifier used a "passw" text match on the request arguments. A secret whose name has no such text passed the check, so a read of an authentication key, a privacy key or the snmpc site value logged its bare value in the response. A command that expands an argument also passed the check: nodels with a table name returns every column of the table, and lsdef returns attributes that the request never names. The daemon also ran redact_password over the whole connection log on each request, so the redactor split at the first request of the connection and the change signal swept the text of earlier requests and responses. Add secret_in_request. The routine reports a request that names a secret attribute, selects a secret site key, or dumps a table that owns a secret column through tabdump or nodels, from the same secret set that the argument redaction uses. The response classifier calls it, so the response of such a request logs as redacted. Add secret_in_response. The routine reports response text that holds "passw" or a secret attribute name in assignment or column form. The response finalizer calls it in place of the bare text match, so an expanded listing that carries an authentication key or a product key logs as redacted even when the request never names it. The lsvm response is the directory entry, whose passwords are positional, so the classifier marks the command itself. Build each request segment alone, redact the segment, and then append it to the connection log. The redactor now always sees the current command, and the change signal covers only the current request. --- xCAT-server/lib/perl/xCAT/xcatd.pm | 29 +++++++++++++++++++++++++++++ xCAT-server/sbin/xcatd | 25 +++++++++++++------------ 2 files changed, 42 insertions(+), 12 deletions(-) diff --git a/xCAT-server/lib/perl/xCAT/xcatd.pm b/xCAT-server/lib/perl/xCAT/xcatd.pm index 331b6956a..ccb279570 100644 --- a/xCAT-server/lib/perl/xCAT/xcatd.pm +++ b/xCAT-server/lib/perl/xCAT/xcatd.pm @@ -532,6 +532,35 @@ my %secret_command_patterns = ( my %secret_command_nocase = map { $_ => 1 } qw(mkvm); my %secret_site_keys = map { $_ => 1 } qw(snmpc); +my %secret_table = map { (split /\./)[0] => 1 } grep { /\./ } @secret_attributes; +$secret_table{site} = 1; + +my $secret_response_pattern = do { + my $names = join '|', map { quotemeta } @secret_attributes; + qr/(?:^|[^\w.])(?:$names)\s*[=:]/; +}; + +sub secret_in_response { + my ($class, $text) = @_; + return 0 unless defined $text; + return 1 if $text =~ /passw/i; + return 1 if $text =~ $secret_response_pattern; + return 0; +} + +sub secret_in_request { + my ($class, $command, $args) = @_; + foreach my $arg (@{ $args || [] }) { + next unless defined $arg; + return 1 if defined $command and ($command eq 'tabdump' or $command eq 'nodels') and $secret_table{$arg}; + return 1 if $arg =~ /^([\w.]+)/ and $secret_attribute{$1}; + foreach my $selector (split /,/, $arg) { + return 1 if $selector =~ /^(?:site\.)?key=([\w.]+)$/ and $secret_site_keys{$1}; + } + } + return 0; +} + sub redact_password_arg { my ($class, $arg) = @_; return $arg unless defined $arg; diff --git a/xCAT-server/sbin/xcatd b/xCAT-server/sbin/xcatd index 29fe6f581..f9c99bd4d 100755 --- a/xCAT-server/sbin/xcatd +++ b/xCAT-server/sbin/xcatd @@ -2864,13 +2864,13 @@ sub service_connection { my $strhour = ($hour > 9 ? $hour : "0" . $hour); my $strmin = ($min > 9 ? $min : "0" . $min); my $strsec = ($sec > 9 ? $sec : "0" . $sec); - $cmdlog_alllog .= "[Date] $year-$strmon-$strmday $strhour:$strmin:$strsec\n"; + my $cmdlog_request = "[Date] $year-$strmon-$strmday $strhour:$strmin:$strsec\n"; - $cmdlog_alllog .= "[ClientType] " . $req->{clienttype}->[0] . " \n"; - $cmdlog_alllog .= "[Request] " . $req->{command}->[0] . " "; + $cmdlog_request .= "[ClientType] " . $req->{clienttype}->[0] . " \n"; + $cmdlog_request .= "[Request] " . $req->{command}->[0] . " "; if (exists($req->{noderange}) && defined($req->{noderange}->[0])) { my $tmpstr = join(",", @{$req->{noderange}}); - $cmdlog_alllog .= "$tmpstr "; + $cmdlog_request .= "$tmpstr "; } my $cmdlog_req_redacted = 0; @@ -2881,17 +2881,17 @@ sub service_connection { if ($arg =~ /[^A-Za-z0-9.-]/) { my $tmparg = $arg; $tmparg =~ s/'/'\\''/g; - $cmdlog_alllog .= "'" . $tmparg . "' "; + $cmdlog_request .= "'" . $tmparg . "' "; } else { - $cmdlog_alllog .= $arg . " "; + $cmdlog_request .= $arg . " "; } } } # Replace passwords with 'x' - my $cmdlog_before_redact = $cmdlog_alllog; - $cmdlog_alllog = xCAT::xcatd->redact_password($cmdlog_alllog); - $cmdlog_req_redacted = 1 if $cmdlog_alllog ne $cmdlog_before_redact; - $cmdlog_alllog .= "\n[Response]\n"; + my $cmdlog_before_redact = $cmdlog_request; + $cmdlog_request = xCAT::xcatd->redact_password($cmdlog_request); + $cmdlog_req_redacted = 1 if $cmdlog_request ne $cmdlog_before_redact; + $cmdlog_alllog .= $cmdlog_request . "\n[Response]\n"; $cmdlog_response_sensitive = cmdlog_response_is_sensitive($req, $cmdlog_req_redacted); # ----used for command log end---------- @@ -3630,14 +3630,15 @@ sub cmdlog_collectlog() { sub cmdlog_response_is_sensitive { my ($req, $req_redacted) = @_; - return 1 if defined $req->{command}->[0] and $req->{command}->[0] eq 'getcredentials'; + return 1 if defined $req->{command}->[0] and ($req->{command}->[0] eq 'getcredentials' or $req->{command}->[0] eq 'lsvm'); return 1 if join(' ', @{ $req->{arg} || [] }) =~ /passw/i; + return 1 if xCAT::xcatd->secret_in_request($req->{command}->[0], $req->{arg}); return 1 if $req_redacted; return 0; } sub cmdlog_finalize_response { - if ($cmdlog_response_sensitive or $cmdlog_response_buffer =~ /passw/i) { + if ($cmdlog_response_sensitive or xCAT::xcatd->secret_in_response($cmdlog_response_buffer)) { $cmdlog_response_buffer = "*REDACTED*\n"; } $cmdlog_alllog .= $cmdlog_response_buffer; From 5fb762f2b6096e63e674efe97701b7f53b560d05 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vin=C3=ADcius=20Ferr=C3=A3o?= <2031761+viniciusferrao@users.noreply.github.com> Date: Thu, 20 Aug 2026 19:37:57 -0300 Subject: [PATCH 2/2] test(xcatd): cover the secret set response classification Extract secret_in_request, secret_in_response and the secret sets from xcatd.pm, because the classifier and the finalizer consult them. Cover the authentication key, the privacy key and the snmpc site value reads as sensitive, the token, prodkey and site table dumps as sensitive, the nodels expansion of a secret table as sensitive, the lsvm directory listing as sensitive, and the implicit lsdef attribute listing as redacted. Keep a plain site value, a benign table, a benign object listing and a group named like a table as not sensitive. Assert that the daemon redacts each request segment alone. --- xCAT-test/unit/cmdlog_response_redact.t | 52 +++++++++++++++++++++++-- 1 file changed, 48 insertions(+), 4 deletions(-) diff --git a/xCAT-test/unit/cmdlog_response_redact.t b/xCAT-test/unit/cmdlog_response_redact.t index e79f4cd85..4bcc7054e 100644 --- a/xCAT-test/unit/cmdlog_response_redact.t +++ b/xCAT-test/unit/cmdlog_response_redact.t @@ -3,14 +3,18 @@ use strict; use warnings; use FindBin; +use lib "$FindBin::Bin/../lib"; +use lib "$FindBin::Bin/../../perl-xCAT"; +use lib "$FindBin::Bin/../../xCAT-server/lib/perl"; use Test::More; -my $xcatd = "$FindBin::Bin/../../xCAT-server/sbin/xcatd"; +use XCAT::Test::File qw(repo_path slurp_repo_file); +use xCAT::xcatd; + +my $xcatd = repo_path('xCAT-server/sbin/xcatd'); plan skip_all => 'xcatd not found' unless -r $xcatd; -open my $fh, '<', $xcatd or die $!; -my $src = do { local $/; <$fh> }; -close $fh; +my $src = slurp_repo_file('xCAT-server/sbin/xcatd'); # Extract the three command-log response subs and load them. xcatd is present, # so a sub that cannot be extracted is a hard failure, not a skip. @@ -36,6 +40,8 @@ die "eval of command-log subs failed: $@" if $@; # Classification from the request. ok(cmdlog_response_is_sensitive({ command => ['getcredentials'], arg => [] }, ''), 'getcredentials is a sensitive-response command'); +ok(cmdlog_response_is_sensitive({ command => ['lsvm'], arg => [] }, 0), + 'lsvm returns the directory entry with its passwords, so it is sensitive'); ok(cmdlog_response_is_sensitive({ command => ['gettab'], arg => ['key=xcat', 'passwd.password'] }, ''), 'gettab of a passwd column is sensitive'); ok(cmdlog_response_is_sensitive({ command => ['tabdump'], arg => ['passwd'] }, ''), @@ -45,6 +51,35 @@ ok(cmdlog_response_is_sensitive({ command => ['rspconfig'], arg => [] }, 1), ok(!cmdlog_response_is_sensitive({ command => ['rpower'], arg => ['n1', 'stat'] }, 0), 'a benign request is not sensitive'); +# A secret attribute with no "passw" in its name must classify through the +# shared secret set, not the text heuristic. +ok(cmdlog_response_is_sensitive({ command => ['gettab'], arg => ['node=pdu01', 'pdu.authkey'] }, 0), + 'gettab of an authentication key is sensitive'); +ok(cmdlog_response_is_sensitive({ command => ['gettab'], arg => ['node=pdu01', 'pdu.privkey'] }, 0), + 'gettab of a privacy key is sensitive'); +ok(cmdlog_response_is_sensitive({ command => ['gettab'], arg => ['key=snmpc', 'site.value'] }, 0), + 'gettab of the snmpc site value is sensitive'); +ok(!cmdlog_response_is_sensitive({ command => ['gettab'], arg => ['key=domain', 'site.value'] }, 0), + 'gettab of a plain site value is not sensitive'); + +# A dump of a whole table that owns a secret column returns the bare values. +ok(cmdlog_response_is_sensitive({ command => ['tabdump'], arg => ['token'] }, 0), + 'tabdump of the token table is sensitive'); +ok(cmdlog_response_is_sensitive({ command => ['tabdump'], arg => ['prodkey'] }, 0), + 'tabdump of the prodkey table is sensitive'); +ok(cmdlog_response_is_sensitive({ command => ['tabdump'], arg => ['site'] }, 0), + 'tabdump of the site table is sensitive'); +ok(cmdlog_response_is_sensitive({ command => ['tabdump'], arg => ['-w', 'key==snmpc', 'site'] }, 0), + 'a filtered site dump is sensitive'); +ok(!cmdlog_response_is_sensitive({ command => ['tabdump'], arg => ['networks'] }, 0), + 'tabdump of the networks table is not sensitive'); +ok(cmdlog_response_is_sensitive({ command => ['nodels'], noderange => ['node01'], arg => ['prodkey'] }, 0), + 'nodels of a whole secret table is sensitive'); +ok(!cmdlog_response_is_sensitive({ command => ['nodels'], noderange => ['switches'], arg => [] }, 0), + 'a group named like a secret table is not sensitive'); +ok(!cmdlog_response_is_sensitive({ command => ['nodels'], noderange => ['node01'], arg => ['nodetype'] }, 0), + 'nodels of a benign table is not sensitive'); + # Drive collect(s) then finalize, returning what was appended to the log. sub run { my ($sensitive, @responses) = @_; @@ -77,6 +112,15 @@ my $benign = run(0, "node01: on"); like($benign, qr/node01: on/, 'a benign response is logged verbatim'); unlike($benign, qr/\*REDACTED\*/, 'a benign response is not redacted'); +# A detailed object listing expands attributes the request never named. +my $lsdef = run(0, 'Object name: pdu01', ' authkey=AUTH_SECRET', ' privkey=PRIV_SECRET'); +unlike($lsdef, qr/AUTH_SECRET|PRIV_SECRET/, 'implicit lsdef secrets are not logged'); +like($lsdef, qr/\*REDACTED\*/, 'lsdef response containing secret attributes is redacted'); +my $colon = run(0, 'node01: prodkey.key: AAAAA-BBBBB'); +unlike($colon, qr/AAAAA/, 'a colon separated secret column is redacted'); +my $plain = run(0, 'Object name: node01', ' groups=compute', ' mgt=ipmi'); +unlike($plain, qr/\*REDACTED\*/, 'a benign object listing is not redacted'); + # Two requests on one connection: the finalizer runs at the next command's # start. The earlier response must be preserved (redacted), the flag reset, and # the next benign response neither lost nor over-redacted.