Merge pull request #7719 from VersatusHPC/fix/redact-password-attributes

fix(xcatd): redact node passwords from the command logs

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;

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();