diff --git a/xCAT-server/lib/perl/xCAT/xcatd.pm b/xCAT-server/lib/perl/xCAT/xcatd.pm index bfe235174..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); @@ -562,6 +568,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; 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();