From 7fa755719a13f37f5c4e6092833c58e5695757de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vin=C3=ADcius=20Ferr=C3=A3o?= <2031761+viniciusferrao@users.noreply.github.com> Date: Wed, 19 Aug 2026 13:43:23 -0300 Subject: [PATCH 1/2] fix(xcatd): redact command-log arguments per element The daemon redacted secret attributes on the joined command string. The match failed when a value held a space. The match also failed for a "+=" splice assignment. The validate() path did not quote the arguments, so a multi-word secret value kept its later words in syslog and in the auditlog table. A password that a command receives through an option or a positional operand was not redacted at all. The debug dispatch trace wrote the raw arguments to syslog when site.xcatdebugmode was set. Redact the argument vector before the daemon joins it. Add redact_password_args for this task. The routine masks the value of a secret attribute in any argument, at the start or embedded after another token. An embedded secret assignment masks to the end of the argument, because a shell value may hold quotes and spaces. The routine allows spaces around the operator. It accepts the "=", "+=", ",=", "^=", "!=", "=~" and "!~" operators that chdef, nodech and node selection use. It masks a password option value in each form that Getopt::Long accepts: a separate argument, a compact short option, a bundle of short options with the "?" help letter, a "+" option prefix, a single-letter option with two dashes, a long option, a long option with an equals sign, and an abbreviated long option. The long-name match runs first, so a long option keeps its name and masks its value. A walk over each bundle then finds the first secret letter, so the mask always starts at the option and the result does not depend on hash order. The walk knows which other letters of a command take a value, so a secret letter inside such a value does not redact and the audit text stays correct. The walk also knows which letters take an integer, because the z/VM cpu option consumes only its signed digits and the parser then continues the bundle into the password option. The value stops match letter case, because a bundle keeps short options case sensitive and an unknown capital letter does not absorb the rest. The mkvm secret match ignores letter case, because the z/VM parser keeps the Getopt::Long default for long names. The mkhwconn match keeps letter case, because -p is the hardware control point and -P is the password. The routine knows the password options of bmcdiscover, switchdiscover, mkhwconn, mkvm, createvcluster, lsvcluster and rmvcluster, the rspconfig password assignments, the mkvm clone pw= operand, and the positional password operands of chvm. It masks the site.value argument of tabch and chtab when a selector or a site.key assignment names snmpc. An exact short option that takes a non-secret value stays visible, so the PPC mkvm -p profile is not an abbreviation of --password. The dispatch trace builds its text from the redacted vector. Add snmpc, productkey, prodkey.key, tokenid and token.tokenid to the secret list, with community and pdu.community. The secret list holds only attributes that map to a secret column, so key and sshkeydir stay visible. redact_password keeps a second pass over the joined string. This pass masks an embedded secret assignment to the end of the line, because the argument boundaries are gone after the join. The commands.log response classifier marks a response sensitive when the request was redacted. The argument vector pass sets that signal, so a secret whose name has no "passw" text still marks its response. --- xCAT-server/lib/perl/xCAT/xcatd.pm | 218 +++++++++++++++++++++++------ xCAT-server/sbin/xcatd | 10 +- 2 files changed, 180 insertions(+), 48 deletions(-) diff --git a/xCAT-server/lib/perl/xCAT/xcatd.pm b/xCAT-server/lib/perl/xCAT/xcatd.pm index 3bafeeb83..331b6956a 100644 --- a/xCAT-server/lib/perl/xCAT/xcatd.pm +++ b/xCAT-server/lib/perl/xCAT/xcatd.pm @@ -244,7 +244,8 @@ sub validate { # add each argument my $args = $request->{arg}; my $arglist; - foreach my $argument (@$args) { + my ($redacted_args) = xCAT::xcatd->redact_password_args($request->{command}->[0], $args); + foreach my $argument (@$redacted_args) { $arglist .= " " . $argument; } my $saveArglist = $arglist; @@ -484,6 +485,174 @@ sub verifytoken { return undef; } } +# -------------------------------------------------------------------------------- +my @secret_attributes = qw( + authkey bmcpassword domainadminpassword iscsipassword + passwd.HMC passwd.admin passwd.celogin passwd.general passwd.hscroot + password privkey snmppassword snmpc community productkey tokenid + domain.adminpassword ipmi.password iscsi.passwd mpa.password + openbmc.password passwd.password pdu.authkey pdu.community pdu.password + pdu.privkey ppcdirect.password ppchcp.password prodkey.key + switches.password switches.sshpassword token.tokenid vm.vidpassword + websrv.password +); +my %secret_attribute = map { $_ => 1 } @secret_attributes; + +my %secret_command_options = ( + bmcdiscover => [qw(p bmcpasswd n newbmcpw)], + switchdiscover => [qw(c)], + mkhwconn => [qw(P)], + mkvm => [qw(w password)], + createvcluster => [qw(password)], + lsvcluster => [qw(password)], + rmvcluster => [qw(password)], +); +my %secret_command_valueopts = ( + bmcdiscover => 'smiu', + switchdiscover => 's', + mkhwconn => 'psT', + mkvm => 'dijlmpqrvz', +); +my %secret_command_intopts = ( + mkvm => 'c', +); +my %secret_command_operands = ( + chvm => { + '--setpassword' => [1], + '--add3390' => [ 5, 6, 7 ], + '--add9336' => [ 5, 6, 7 ], + '--addpagespool' => [8], + '--formatdisk' => [2], + }, +); +my %secret_command_patterns = ( + mkvm => qr/^(pw=)/, + rspconfig => qr/^(\*_passwd=|admin_passwd=|HMC_passwd=|general_passwd=|USERID=)/, +); +my %secret_command_nocase = map { $_ => 1 } qw(mkvm); +my %secret_site_keys = map { $_ => 1 } qw(snmpc); + +sub redact_password_arg { + my ($class, $arg) = @_; + return $arg unless defined $arg; + if ($arg =~ /^\s*([\w.]+)\s*([-+,^!]?=~?|!~)/ and $secret_attribute{$1}) { + return $1 . $2 . "xxxxxxxx"; + } + while ($arg =~ /(? 1 } @option; + my $valueopts = (defined $command and $secret_command_valueopts{$command}) + ? $secret_command_valueopts{$command} : ''; + my $intopts = (defined $command and $secret_command_intopts{$command}) + ? $secret_command_intopts{$command} : ''; + if ($nocase) { $letters = lc $letters; } + my $pattern = defined $command ? $secret_command_patterns{$command} : undef; + my %operand; + if (defined $command and $secret_command_operands{$command} + and defined $args->[0] + and $secret_command_operands{$command}{ $args->[0] }) { + %operand = map { $_ => 1 } @{ $secret_command_operands{$command}{ $args->[0] } }; + } + my $sitevalue = 0; + if (defined $command and ($command eq 'tabch' or $command eq 'chtab')) { + SELECTOR: foreach my $selectorarg (@$args) { + next unless defined $selectorarg; + foreach my $selector (split /,/, $selectorarg) { + if ($selector =~ /^(?:site\.)?key=([\w.]+)$/ and $secret_site_keys{$1}) { + $sitevalue = 1; + last SELECTOR; + } + } + } + } + for my $index (0 .. $#{$args}) { + my $arg = $args->[$index]; + if (not defined $arg) { push @redacted, $arg; next; } + if ($pending) { + $pending = 0; + $changed = 1; + push @redacted, "xxxxxxxx"; + next; + } + if ($operand{$index}) { + $changed = 1; + push @redacted, "xxxxxxxx"; + next; + } + my $masked; + foreach my $name (@longnames) { + if ($arg =~ /^(-{1,2}|\+)([A-Za-z]+)(=?)(.*)$/s and index($name, lc($2)) == 0) { + my $short = $nocase ? lc($2) : $2; + next if length($2) == 1 and index($valueopts, $short) >= 0; + if ($3) { $masked = $1 . $2 . $3 . "xxxxxxxx"; last; } + if ($4 eq '') { $pending = 1; last; } + } + } + if (not $pending and not defined $masked and $letters) { + if ($arg =~ /^--([A-Za-z])(.*)$/s) { + my ($c, $rest) = ($1, $2); + my $cc = $nocase ? lc $c : $c; + if (index($letters, $cc) >= 0) { + if ($rest eq '') { $pending = 1; } + else { $masked = "--" . $c . "xxxxxxxx"; } + } + } elsif ($arg =~ /^([-+])([A-Za-z?].*)$/s) { + my ($intro, $body) = ($1, $2); + my $offset = 0; + my $length = length $body; + while ($offset < $length) { + my $c = substr($body, $offset, 1); + last if $c !~ /[A-Za-z?]/; + my $cc = $nocase ? lc $c : $c; + if (index($letters, $cc) >= 0) { + if ($offset == $length - 1) { $pending = 1; } + else { $masked = $intro . substr($body, 0, $offset + 1) . "xxxxxxxx"; } + last; + } + last if index($valueopts, $c) >= 0; + if ($intopts ne '' and index($intopts, $c) >= 0) { + $offset++; + $offset++ if $offset < $length and substr($body, $offset, 1) =~ /[-+]/; + $offset++ while $offset < $length and substr($body, $offset, 1) =~ /[0-9_]/; + next; + } + $offset++; + } + } + } + if (not defined $masked and $pattern and $arg =~ /$pattern/s) { + $masked = $1 . "xxxxxxxx"; + } + if (not defined $masked and $sitevalue and $arg =~ /^(site\.value\s*(?:[-+,^!]?=~?|!~)\s*)/s) { + $masked = $1 . "xxxxxxxx"; + } + if (defined $masked) { + $changed = 1; + push @redacted, $masked; + next; + } + my $attrarg = $class->redact_password_arg($arg); + $changed = 1 if $attrarg ne $arg; + push @redacted, $attrarg; + } + return (\@redacted, $changed); +} + # -------------------------------------------------------------------------------- =head3 redact_password @@ -516,7 +685,6 @@ sub verifytoken { 'HMC_passwd=xxx' '*_passwd=xxxxxxx' =cut -# -------------------------------------------------------------------------------- sub redact_password { my $class = shift; my $request = shift; @@ -568,50 +736,10 @@ 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; + foreach my $attribute (@secret_attributes) { + $parameters =~ s/(^|\s)'(\Q$attribute\E\s*(?:[-+,^!]?=~?|!~)\s*)[^']*'/$1'$2$redact_string'/g; + $parameters =~ s/(^|[^\w.])(\Q$attribute\E\s*(?:[-+,^!]?=~?|!~)\s*)(?!\Q$redact_string\E).*$/$1$2$redact_string/; } - # Return original request with password replaced by 'x' in $parameters string if ($request =~ '\[Request\]') { return $header . "[Request] " . $command . " " . $parameters; diff --git a/xCAT-server/sbin/xcatd b/xCAT-server/sbin/xcatd index 0596d49a3..42c690a9b 100755 --- a/xCAT-server/sbin/xcatd +++ b/xCAT-server/sbin/xcatd @@ -2283,7 +2283,8 @@ sub dispatch_request { } if (exists($req->{arg})) { - foreach my $arg (@{ $req->{arg} }) { + my ($trace_args) = xCAT::xcatd->redact_password_args($req->{command}->[0], $req->{arg}); + foreach my $arg (@$trace_args) { $str_cmd .= $arg . " "; } $str_cmd =~ s/(.+) $/$1/g; @@ -2869,8 +2870,11 @@ sub service_connection { $cmdlog_alllog .= "$tmpstr "; } + my $cmdlog_req_redacted = 0; if (exists($req->{arg})) { - foreach my $arg (@{ $req->{arg} }) { + my $redacted_args; + ($redacted_args, $cmdlog_req_redacted) = xCAT::xcatd->redact_password_args($req->{command}->[0], $req->{arg}); + foreach my $arg (@$redacted_args) { if ($arg =~ /[^A-Za-z0-9.-]/) { my $tmparg = $arg; $tmparg =~ s/'/'\\''/g; @@ -2883,7 +2887,7 @@ sub service_connection { # Replace passwords with 'x' my $cmdlog_before_redact = $cmdlog_alllog; $cmdlog_alllog = xCAT::xcatd->redact_password($cmdlog_alllog); - my $cmdlog_req_redacted = ($cmdlog_alllog ne $cmdlog_before_redact) ? 1 : 0; + $cmdlog_req_redacted = 1 if $cmdlog_alllog ne $cmdlog_before_redact; $cmdlog_alllog .= "\n[Response]\n"; $cmdlog_response_sensitive = cmdlog_response_is_sensitive($req, $cmdlog_req_redacted); From 0d8d1e60ff1179fd12a880e15e0d60ed7197619e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vin=C3=ADcius=20Ferr=C3=A3o?= <2031761+viniciusferrao@users.noreply.github.com> Date: Wed, 19 Aug 2026 13:43:24 -0300 Subject: [PATCH 2/2] test(xcatd): cover per-element command-log redaction Extract the secret set, the command maps and the redaction routines from xcatd.pm. Drive the real routines, because the module needs daemon dependencies that the test host does not have. Cover a multi-word value, a quoted value, spaces around the operator, the "+=", ",=", "^=", "!=", "=~" and "!~" operators, the community strings, a secret embedded in a compound argument with and without quotes, each Getopt::Long password option form with bundles, the "?" help letter, abbreviations, "+" prefixes and two-dash letters, values that hold another secret letter, non-secret compact values that a value option absorbs, long names that start with a secret letter, the mkvm case forms and compact values, the chvm positional passwords, the mkvm clone pw= operand, the vCenter cluster passwords, the snmpc site value through chdef and tabch with compound selectors, the authentication tokens, and the product keys. Assert that validate() redacts the argument vector, that the joined result runs through redact_password, and that the dispatch trace builds its text from redacted arguments. Keep every attribute and column pair from Schema.pm, so a removed table-qualified mapping fails the test. --- xCAT-test/unit/xcatd_password_redaction.t | 320 ++++++++++++++++++---- 1 file changed, 273 insertions(+), 47 deletions(-) diff --git a/xCAT-test/unit/xcatd_password_redaction.t b/xCAT-test/unit/xcatd_password_redaction.t index bf45db751..33ba219fe 100644 --- a/xCAT-test/unit/xcatd_password_redaction.t +++ b/xCAT-test/unit/xcatd_password_redaction.t @@ -8,9 +8,10 @@ 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 $daemon = File::Spec->catfile( $repo_root, 'xCAT-server/sbin/xcatd' ); 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; +plan skip_all => 'xcatd.pm, xcatd or Schema.pm not found' unless -r $xcatd && -r $daemon && -r $schema; sub slurp { open( my $fh, '<', $_[0] ) or die "Unable to read $_[0]: $!"; @@ -20,68 +21,293 @@ sub slurp { } my $source = slurp($xcatd); +my $daemon_source = slurp($daemon); 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: $@"); +# xcatd.pm cannot be loaded here (dependencies), so lift out the secret set, the +# command maps, and the redaction routines and evaluate them alone. +my ($set) = $source =~ /(my \@secret_attributes = qw\(.*?\);\s*my %secret_attribute = map.*?;)/s; +my ($maps) = $source =~ /(my %secret_command_options = \(.*?my %secret_site_keys = map.*?;)/s; +my ($arg_sub) = $source =~ /(sub redact_password_arg \{.*?\n\}\n)/s; +my ($args_sub) = $source =~ /(sub redact_password_args \{.*?\n\}\n)/s; +my ($cmd_sub) = $source =~ /(sub redact_password \{.*?\n\}\n)/s; +BAIL_OUT('could not extract the secret set from xcatd.pm') unless $set; +BAIL_OUT('could not extract the command maps from xcatd.pm') unless $maps; +BAIL_OUT('could not extract redact_password_arg from xcatd.pm') unless $arg_sub; +BAIL_OUT('could not extract redact_password_args from xcatd.pm') unless $args_sub; +BAIL_OUT('could not extract redact_password from xcatd.pm') unless $cmd_sub; +eval "package RedactUnderTest; $set $maps $arg_sub $args_sub $cmd_sub 1;" + or BAIL_OUT("could not evaluate the redaction routines: $@"); -sub redacted { return RedactUnderTest::redact_password( $_[0], $_[1] ); } +sub arg { return RedactUnderTest::redact_password_arg( 'xCAT::xcatd', $_[0] ); } +sub cmd { 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" ); +sub vec_ { + my ( $command, @args ) = @_; + my ( $redacted, $changed ) = + RedactUnderTest->redact_password_args( $command, \@args ); + return ( join( ' ', @$redacted ), $changed ); } -# 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' -); +# A secret in an attribute assignment must never survive, however it was written. +unlike( arg('bmcpassword=SEKRET'), qr/SEKRET/, 'a bare attribute value is redacted' ); +unlike( arg('bmcpassword=SEKRET phrase'), qr/SEKRET|phrase/, 'a value with spaces is redacted whole' ); +unlike( arg("bmcpassword=has'quote"), qr/quote/, 'a value with a quote is redacted whole' ); +unlike( arg('passwd.password+=SEKRET'), qr/SEKRET/, 'a += splice assignment is redacted' ); +unlike( arg('pdu.community=SEKRET'), qr/SEKRET/, 'the SNMP community string is redacted' ); +unlike( arg('community=SEKRET'), qr/SEKRET/, 'a bare community value is redacted' ); -# 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; +# The chdef/mkdef parser trims whitespace around '=', so an attribute may be +# written with spaces. The value must still be redacted. +unlike( arg('bmcpassword = SEKRET'), qr/SEKRET/, 'spaces around the equals are redacted' ); +unlike( arg('bmcpassword =SEKRET'), qr/SEKRET/, 'a space before the equals is redacted' ); +unlike( arg('bmcpassword= SEKRET'), qr/SEKRET/, 'a space after the equals is redacted' ); +unlike( arg('bmcpassword = SEKRET phrase'), qr/SEKRET|phrase/, 'spaces around the equals with a multi-word value are redacted whole' ); + +# nodech and node selection accept operators other than a bare '='. +unlike( arg('ipmi.password,=SEKRET'), qr/SEKRET/, 'a ,= append assignment is redacted' ); +unlike( arg('ipmi.password^=SEKRET'), qr/SEKRET/, 'a ^= remove assignment is redacted' ); +unlike( arg('ipmi.password!=SEKRET'), qr/SEKRET/, 'a != selection is redacted' ); +unlike( arg('ipmi.password!~SEKRET'), qr/SEKRET/, 'a !~ selection is redacted' ); +unlike( arg('ipmi.password=~SEKRET'), qr/SEKRET/, 'a =~ selection is redacted' ); + +# Non-secret detail stays, or the log stops being useful. +is( arg('groups=lab'), 'groups=lab', 'an unrelated attribute is kept' ); +is( arg('key=system'), 'key=system', 'key is kept, it is not a secret' ); +is( arg('sshkeydir=/etc/xcat/keys'), 'sshkeydir=/etc/xcat/keys', 'sshkeydir is kept, it is a directory' ); +is( arg('n1'), 'n1', 'a plain argument is kept' ); + +# The command-flag mechanism still redacts positional password flags. +unlike( cmd( 'bmcdiscover', ' -s nmap -p SEKRET --range 10.0.0.1' ), qr/SEKRET/, + 'a -p flag value is redacted' ); + +# An assignment embedded in a compound argument, for example an xdsh remote +# command string, must be masked to the end of the argument, because a shell +# value may hold quotes and spaces. +unlike( cmd( 'xdsh', " compute 'echo bmcpassword=SEKRET > /etc/x'" ), qr/SEKRET/, + 'a secret embedded in a compound argument is redacted' ); +unlike( cmd( 'xdsh', q{ compute "export password='SEKRET phrase'; run-app"} ), qr/SEKRET|phrase/, + 'a quoted secret embedded in a compound argument is redacted whole' ); +unlike( arg(q{export password='SEKRET phrase'; run-app}), qr/SEKRET|phrase/, + 'a quoted secret inside one argument is redacted to the end' ); +unlike( arg('usercomment=password=SEKRET'), qr/SEKRET/, + 'a secret glued to a prior assignment is redacted' ); +is( arg('echo groups=lab; ls'), 'echo groups=lab; ls', 'an embedded non-secret assignment is kept' ); + +# A password given through a command option must be redacted in every form +# Getopt::Long accepts: a separate argument, a compact short option, a long +# option, and a long option with an equals sign. +my ( $out, $changed ); +( $out, $changed ) = vec_( 'bmcdiscover', '--range', '10.0.0.1', '-p', 'SEKRET phrase' ); +unlike( $out, qr/SEKRET|phrase/, 'a -p value in the next argument is redacted whole' ); +is( $changed, 1, 'the option redaction reports the change' ); +( $out ) = vec_( 'bmcdiscover', '--range', '10.0.0.1', '-pSEKRET' ); +unlike( $out, qr/SEKRET/, 'a compact -pSEKRET is redacted' ); +( $out ) = vec_( 'bmcdiscover', '--range', '10.0.0.1', '--bmcpasswd', 'SEKRET' ); +unlike( $out, qr/SEKRET/, 'a --bmcpasswd value in the next argument is redacted' ); +( $out ) = vec_( 'bmcdiscover', '--range', '10.0.0.1', '--bmcpasswd=SEKRET' ); +unlike( $out, qr/SEKRET/, 'a --bmcpasswd=value is redacted' ); +( $out ) = vec_( 'bmcdiscover', '-n', 'SEKRET' ); +unlike( $out, qr/SEKRET/, 'a -n new password is redacted' ); +( $out ) = vec_( 'bmcdiscover', '--newbmcpw', 'SEKRET' ); +is( $out, '--newbmcpw xxxxxxxx', 'the full --newbmcpw name is kept and its value is masked' ); +( $out ) = vec_( 'mkvm', 'zvm02', '-password', 'SEKRET' ); +is( $out, 'zvm02 -password xxxxxxxx', 'a single-dash long -password keeps its name and masks its value' ); +( $out ) = vec_( 'switchdiscover', '--range', '10.0.0.0/24', '-c', 'SEKRET' ); +unlike( $out, qr/SEKRET/, 'the switchdiscover -c community is redacted' ); +( $out ) = vec_( 'mkhwconn', 'frame', '-p', 'hmc01', '-PSEKRET' ); +unlike( $out, qr/SEKRET/, 'the mkhwconn -P password is redacted' ); +like( $out, qr/-p hmc01/, 'the mkhwconn -p hardware control point is kept' ); +( $out ) = vec_( 'mkvm', 'zvm02', '--password', 'SEKRET' ); +unlike( $out, qr/SEKRET/, 'the mkvm --password value is redacted' ); +( $out ) = vec_( 'rspconfig', 'admin_passwd=SEKRET phrase' ); +unlike( $out, qr/SEKRET|phrase/, 'a rspconfig password assignment is redacted whole' ); + +# Getopt::Long also accepts an abbreviated long option, a bundle of short +# options, and, for parsers that keep the default configuration, any letter +# case. Each of those forms must redact too. +( $out ) = vec_( 'bmcdiscover', '--range', '10.0.0.1', '--bmcp', 'SEKRET' ); +unlike( $out, qr/SEKRET/, 'an abbreviated --bmcp is redacted' ); +( $out ) = vec_( 'bmcdiscover', '--range', '10.0.0.1', '--bmcpas=SEKRET' ); +unlike( $out, qr/SEKRET/, 'an abbreviated --bmcpas=value is redacted' ); +( $out ) = vec_( 'bmcdiscover', '--range', '10.0.0.1', '-zp', 'SEKRET' ); +unlike( $out, qr/SEKRET/, 'a bundled -zp is redacted' ); +( $out ) = vec_( 'bmcdiscover', '--range', '10.0.0.1', '-zpSEKRET' ); +unlike( $out, qr/SEKRET/, 'a bundled compact -zpSEKRET is redacted' ); +( $out ) = vec_( 'bmcdiscover', '-pApple' ); +is( $out, '-pxxxxxxxx', 'a compact value keeps no leading letters' ); +( $out ) = vec_( 'bmcdiscover', '-pAdmin', '--range', '10.0.0.1' ); +is( $out, '-pxxxxxxxx --range 10.0.0.1', 'a compact value ending in a secret letter is masked, not the next argument' ); +( $out ) = vec_( 'bmcdiscover', '-nstop', '--range', '10.0.0.1' ); +is( $out, '-nxxxxxxxx --range 10.0.0.1', 'a compact -n value is masked whole' ); +( $out ) = vec_( 'bmcdiscover', '-pbanana' ); +is( $out, '-pxxxxxxxx', 'a compact value with a later secret letter is masked from the option' ); +( $out ) = vec_( 'bmcdiscover', '--p=SEKRET' ); +unlike( $out, qr/SEKRET/, 'a double-dash --p=value is redacted' ); +( $out ) = vec_( 'switchdiscover', '--c', 'SEKRET' ); +unlike( $out, qr/SEKRET/, 'a double-dash --c community is redacted' ); +( $out ) = vec_( 'mkhwconn', 'frame', '--P=SEKRET' ); +unlike( $out, qr/SEKRET/, 'a double-dash --P=value is redacted' ); +( $out, $changed ) = vec_( 'mkhwconn', 'frame', '--p', 'hmc01' ); +is( $changed, 0, 'the double-dash --p control point is kept, the case differs' ); +( $out ) = vec_( 'mkvm', 'zvm02', '--W', 'SEKRET' ); +unlike( $out, qr/SEKRET/, 'a double-dash --W is redacted, the parser ignores case' ); + +# The vCenter cluster commands log in with a --password option. +( $out ) = vec_( 'createvcluster', '--vcenter', 'vc01', '--username', 'admin', '--password', 'SEKRET', 'cluster01' ); +unlike( $out, qr/SEKRET/, 'the createvcluster password is redacted' ); +like( $out, qr/--username admin/, 'the createvcluster username is kept' ); +( $out ) = vec_( 'lsvcluster', '--vcenter', 'vc01', '--password=SEKRET' ); +unlike( $out, qr/SEKRET/, 'the lsvcluster password is redacted' ); +( $out ) = vec_( 'rmvcluster', '--vcenter', 'vc01', '--PASSWORD', 'SEKRET', 'cluster01' ); +unlike( $out, qr/SEKRET/, 'the rmvcluster password is redacted, the parser ignores case' ); + +# In a bundle a non-secret option that takes a value absorbs the rest, so a +# secret letter inside that value must not redact. +( $out, $changed ) = vec_( 'bmcdiscover', '-snmap', '--range', '10.0.0.0/24' ); +is( $out, '-snmap --range 10.0.0.0/24', 'the -snmap scan method is kept, n is the value of -s' ); +is( $changed, 0, 'a non-secret compact value reports no change' ); +( $out, $changed ) = vec_( 'bmcdiscover', '-sopenbmc' ); +is( $out, '-sopenbmc', 'the -sopenbmc scan method is kept, p is inside the value' ); +is( $changed, 0, 'the -sopenbmc value reports no change' ); +( $out, $changed ) = vec_( 'mkhwconn', 'frame', '-pPOWERHMC' ); +is( $changed, 0, 'the mkhwconn -p value is kept, P is inside the value of -p' ); +( $out, $changed ) = vec_( 'bmcdiscover', '-zsp', 'SEKRET' ); +is( $changed, 0, 'a bundle stops at a value option, -zsp is the value p of -s' ); +( $out ) = vec_( 'bmcdiscover', '-zps', 'topsecret' ); +is( $out, '-zpxxxxxxxx topsecret', 'a bundle masks from the secret letter, the next argument is positional' ); +( $out ) = vec_( 'bmcdiscover', '-?pSEKRET' ); +unlike( $out, qr/SEKRET/, 'a bundle with the ? help letter is redacted' ); +( $out ) = vec_( 'bmcdiscover', '-?n', 'SEKRET' ); +unlike( $out, qr/SEKRET/, 'a -?n bundle masks the next argument' ); +( $out, $changed ) = vec_( 'mkvm', 'lpar01', '-cpower10', '-pprofile1' ); +is( $out, 'lpar01 -cpower10 -pprofile1', 'the mkvm compact values are kept, w is inside the value of -c' ); +is( $changed, 0, 'the mkvm compact values report no change' ); +( $out, $changed ) = vec_( 'mkvm', 'lpar01', '-p', 'profile1' ); +is( $out, 'lpar01 -p profile1', 'the PPC mkvm profile remains visible' ); +is( $changed, 0, 'the PPC mkvm profile reports no redaction' ); + +# In the z/VM grammar -s is a boolean and -c takes an integer, so a bundle can +# continue into the real password option. +( $out ) = vec_( 'mkvm', 'zvm02', '-swSEKRET' ); +unlike( $out, qr/SEKRET/, 'zVM -s followed by bundled -w is redacted' ); +( $out ) = vec_( 'mkvm', 'zvm02', '-sw', 'SEKRET' ); +unlike( $out, qr/SEKRET/, 'zVM bundled -sw masks the next argument' ); +( $out ) = vec_( 'mkvm', 'zvm02', '-c1wSEKRET' ); +unlike( $out, qr/SEKRET/, 'zVM resumes after the numeric -c value and redacts -w' ); +( $out ) = vec_( 'mkvm', 'zvm02', '-c1w', 'SEKRET' ); +unlike( $out, qr/SEKRET/, 'zVM numeric bundle masks the following password argument' ); +( $out, $changed ) = vec_( 'mkvm', 'zvm02', '-c12' ); +is( $changed, 0, 'a plain numeric cpu bundle is kept' ); + +# Bundled short options keep letter case, so a capital letter is unknown to +# the parser and the bundle continues into the password option. +( $out ) = vec_( 'mkvm', 'zvm02', '-Rw', 'SEKRET' ); +unlike( $out, qr/SEKRET/, 'a capital -Rw bundle masks the next argument' ); +( $out ) = vec_( 'mkvm', 'zvm02', '-RwSEKRET' ); +unlike( $out, qr/SEKRET/, 'a glued capital -RwSEKRET is redacted' ); + +# A bundled integer value may carry a sign or underscores. +( $out ) = vec_( 'mkvm', 'zvm02', '-c+1w', 'SEKRET' ); +unlike( $out, qr/SEKRET/, 'a signed cpu value still reaches the password option' ); +( $out ) = vec_( 'mkvm', 'zvm02', '-c-1wSEKRET' ); +unlike( $out, qr/SEKRET/, 'a negative cpu value still reaches the password option' ); +( $out ) = vec_( 'mkvm', 'zvm02', '-c1_0w', 'SEKRET' ); +unlike( $out, qr/SEKRET/, 'an underscored cpu value still reaches the password option' ); +( $out, $changed ) = vec_( 'mkvm', 'zvm02', '-rabcw', 'KEEPME' ); +is( $changed, 0, 'a lowercase -r value absorbs the rest of the bundle' ); + +# Getopt::Long compatibility mode also accepts "+" as an option starter. +( $out ) = vec_( 'bmcdiscover', '+p', 'SEKRET', '--range', '10.0.0.1' ); +unlike( $out, qr/SEKRET/, 'a +p option is redacted' ); +( $out ) = vec_( 'bmcdiscover', '+bmcpasswd=SEKRET' ); +unlike( $out, qr/SEKRET/, 'a +bmcpasswd=value is redacted' ); +( $out ) = vec_( 'mkvm', 'zvm02', '+w', 'SEKRET' ); +unlike( $out, qr/SEKRET/, 'a +w option is redacted' ); +( $out ) = vec_( 'switchdiscover', '--range', '10.0.0.0/24', '-xc', 'SEKRET' ); +unlike( $out, qr/SEKRET/, 'a bundled -xc community is redacted' ); +( $out ) = vec_( 'mkhwconn', 'frame', '-tP', 'SEKRET' ); +unlike( $out, qr/SEKRET/, 'a bundled -tP password is redacted' ); +( $out, $changed ) = vec_( 'mkhwconn', 'frame', '-p', 'hmc01' ); +is( $out, 'frame -p hmc01', 'the mkhwconn -p stays visible, it is not the password' ); +is( $changed, 0, 'the mkhwconn hardware control point reports no change' ); +( $out ) = vec_( 'mkvm', 'zvm02', '-W', 'SEKRET' ); +unlike( $out, qr/SEKRET/, 'the mkvm -W is redacted, the parser ignores case' ); +( $out ) = vec_( 'mkvm', 'zvm02', '--PASSWORD=SEKRET' ); +unlike( $out, qr/SEKRET/, 'the mkvm --PASSWORD=value is redacted' ); +( $out ) = vec_( 'mkvm', 'zvm02', '--Pass', 'SEKRET' ); +unlike( $out, qr/SEKRET/, 'the mkvm abbreviated --Pass is redacted' ); +( $out ) = vec_( 'mkvm', 'gpok4', 'gpok3', 'pool=POOL1', 'pw=SEKRET' ); +unlike( $out, qr/SEKRET/, 'the mkvm clone pw= operand is redacted' ); +like( $out, qr/pool=POOL1/, 'the mkvm clone pool= operand is kept' ); + +# chvm carries passwords as positional operands. +( $out ) = vec_( 'chvm', '--setpassword', 'SEKRET' ); +unlike( $out, qr/SEKRET/, 'the chvm --setpassword operand is redacted' ); +( $out ) = vec_( 'chvm', '--add3390', 'POOL1', '0101', '3g', 'MR', 'RSEKRET', 'WSEKRET', 'MSEKRET' ); +unlike( $out, qr/SEKRET/, 'the chvm --add3390 disk passwords are redacted' ); +like( $out, qr/POOL1 0101 3g MR/, 'the chvm --add3390 disk parameters are kept' ); +( $out ) = vec_( 'chvm', '--formatdisk', '0100', 'SEKRET' ); +unlike( $out, qr/SEKRET/, 'the chvm --formatdisk password is redacted' ); +like( $out, qr/--formatdisk 0100/, 'the chvm --formatdisk address is kept' ); + +# The site table stores the global SNMP community string under the snmpc key. +( $out ) = vec_( 'tabch', 'key=snmpc', 'site.value=SEKRET' ); +unlike( $out, qr/SEKRET/, 'a tabch of the snmpc site value is redacted' ); +( $out ) = vec_( 'tabch', 'key=snmpc', 'site.value+=SEKRET' ); +unlike( $out, qr/SEKRET/, 'a tabch += splice of the snmpc site value is redacted' ); +( $out ) = vec_( 'tabch', 'key=snmpc,key=snmpc', 'site.value=SEKRET' ); +unlike( $out, qr/SEKRET/, 'a compound tabch selector with snmpc is redacted' ); +( $out ) = vec_( 'tabch', 'key=temporary', 'site.key=snmpc', 'site.value=SEKRET' ); +unlike( $out, qr/SEKRET/, 'a site.key=snmpc assignment marks the value secret' ); +( $out ) = vec_( 'tabch', 'site.key=snmpc', 'site.value=SEKRET' ); +unlike( $out, qr/SEKRET/, 'a table-qualified snmpc selector is redacted' ); +( $out, $changed ) = vec_( 'tabch', 'key=domain', 'site.value=lab' ); +is( $out, 'key=domain site.value=lab', 'a tabch of a plain site value is kept' ); +is( $changed, 0, 'a benign vector reports no change' ); +unlike( arg('snmpc=SEKRET'), qr/SEKRET/, 'the snmpc site attribute is redacted' ); + +# Product keys are license secrets. +unlike( arg('productkey=SEKRET'), qr/SEKRET/, 'a product key is redacted' ); +unlike( arg('prodkey.key=SEKRET'), qr/SEKRET/, 'a table-qualified product key is redacted' ); + +# The token table stores bearer credentials under tokenid. +unlike( arg('tokenid=SEKRET'), qr/SEKRET/, 'an authentication token is redacted' ); +unlike( arg('token.tokenid=SEKRET'), qr/SEKRET/, 'a table-qualified token is redacted' ); + +# Every attribute and column Schema.pm marks secret must be covered. Keep every +# (attribute, column) pair so a removed table-qualified column is caught, not +# masked by another pair that shares the attribute name. +my @pairs; 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; + next + unless $tabentry =~ /\.(password|passwd|authkey|privkey|adminpassword|sshpassword|community)$/i + or $tabentry eq 'prodkey.key'; + push @pairs, [ $attr, $tabentry ]; } -ok( scalar( keys %expected ) > 0, 'Schema.pm yielded attributes mapped to secret columns' ) +ok( scalar(@pairs) > 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/; +foreach my $pair (@pairs) { + my ( $attr, $column ) = @$pair; + push @uncovered, $attr if arg("$attr=SEKRET") =~ /SEKRET/; + push @uncovered, $column if arg("$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' ); +# validate() must redact the argument vector and still run the joined result +# through redact_password, so every secret reaches syslog and the auditlog +# table redacted. +like( $source, qr/=\s*xCAT::xcatd->redact_password_args\(\$request->\{command\}->\[0\]/, 'validate() redacts the argument vector' ); +like( $source, qr/\$redacted_arglist\s*=\s*redact_password\b/, 'validate() redacts the arguments through redact_password' ); + +# The debug dispatch trace must not rebuild the command from the raw request. +like( $daemon_source, qr/\(\$trace_args\)\s*=\s*xCAT::xcatd->redact_password_args\(\$req->\{command\}->\[0\]/, + 'the dispatch trace builds its text from redacted arguments' ); + +# The auditlog table and syslog use the same redacted arguments. like( $source, qr/\$rsp->\{args\}->\[0\]\s*=\s*\$redacted_arglist/, 'the auditlog table stores the redacted arguments' ); done_testing();