Log redaction for sensitive input values, Perl side

This commit attempts to do the same as the C log redaction fix,
but now at the Perl level.  The Perl configuration code was a
little more crufty than the C side, so an additional feature was
added to Config.pm to support the new 'shared' section.  At some
point we should consider a ground-up rewrite of Config.pm, as the
code seems to suffer some from its INI file roots.

Signed-off-by: Dan Wells <dbw2@calvin.edu>
Signed-off-by: Dan Scott <dan@coffeecode.net>

diff --git a/examples/opensrf_core.xml.example b/examples/opensrf_core.xml.example
index a4367d3..5e347e1 100644 (file)
--- a/examples/opensrf_core.xml.example
+++ b/examples/opensrf_core.xml.example
@@ -163,12 +163,12 @@ vim:et:ts=2:sw=2:
     <!-- Any methods which match any of these match_string node values will
          have their params redacted from lower-level input logging.
          Adjust these examples as needed. -->
-    <!--
     <log_protect>
-      <match_string>open-ils.auth_proxy.login</match_string>
+    <!--
+      <match_string>open-ils.auth</match_string>
       <match_string>open-ils.some_service.some_method</match_string>
-    </log_protect>
     -->
+    </log_protect>
   </shared>
 
 </config>

diff --git a/src/perl/lib/OpenSRF/Application.pm b/src/perl/lib/OpenSRF/Application.pm
index 6ebfe4b..93cb92d 100644 (file)
--- a/src/perl/lib/OpenSRF/Application.pm
+++ b/src/perl/lib/OpenSRF/Application.pm
@@ -22,6 +22,7 @@ $log = 'OpenSRF::Utils::Logger';
 
 our $in_request = 0;
 our @pending_requests;
+our $shared_conf;
 
 sub package {
        my $self = shift;
@@ -127,7 +128,26 @@ sub handler {
         my @p = $app_msg->params;
                my $method_name = $app_msg->method;
                my $method_proto = $session->last_message_api_level;
-               $log->info("CALL: ".$session->service." $method_name ". (@p ? join(', ',@p) : ''));
+
+               # By default, we log all method params at the info level
+               # Here we are consult our shared portion of the config file
+               # to look for any exceptions to this behavior
+               my $logdata = "CALL: ".$session->service." $method_name ";
+               my $redact_params = 0;
+               if (@p) {
+                       foreach my $match_string (@{$shared_conf->shared->log_protect}) {
+                               if ($method_name =~ /^$match_string/) {
+                                       $redact_params = 1;
+                                       last;
+                               }
+                       }
+                       if ($redact_params) {
+                               $logdata .= "**PARAMS REDACTED**";
+                       } else {
+                               $logdata .= join(', ',@p);
+                       }
+               }
+               $log->info($logdata);
 
                my $coderef = $app->method_lookup( $method_name, $method_proto, 1, 1 );
 

diff --git a/src/perl/lib/OpenSRF/System.pm b/src/perl/lib/OpenSRF/System.pm
index 7fd7195..62a17a8 100644 (file)
--- a/src/perl/lib/OpenSRF/System.pm
+++ b/src/perl/lib/OpenSRF/System.pm
@@ -84,6 +84,15 @@ sub run_service {
     OpenSRF::Application->application_implementation->initialize()
         if (OpenSRF::Application->application_implementation->can('initialize'));
 
+    # Read in a shared portion of the config file
+    # for later use in log parameter redaction
+    $OpenSRF::Application::shared_conf = OpenSRF::Utils::Config->load(
+        'config_file' => OpenSRF::Utils::Config->current->FILE,
+        'nocache' => 1,
+        'force' => 1,
+        'base_path' => '/config/shared'
+    );
+
     # kill the temp connection
     OpenSRF::Transport::PeerHandle->retrieve->disconnect;
     

diff --git a/src/perl/lib/OpenSRF/Utils/Config.pm b/src/perl/lib/OpenSRF/Utils/Config.pm
index 9ecfc0e..5553dfb 100644 (file)
--- a/src/perl/lib/OpenSRF/Utils/Config.pm
+++ b/src/perl/lib/OpenSRF/Utils/Config.pm
@@ -28,6 +28,8 @@ sub new {
 
        $self->_sub_builder('__id');
        # Hard-code this to match old bootstrap.conf section name
+       # This hardcoded value is later overridden if the config is loaded
+       # with the 'base_path' option
        $self->__id('bootstrap');
 
        my $bootstrap = shift;
@@ -135,9 +137,6 @@ XML elements are pushed into arrays and added as an array reference to
 the hash. Scalar values have whitespace trimmed from the left and
 right sides.
 
-Child elements of C<< <config> >> other than C<< <opensrf> >> are
-currently ignored by this module.
-
 =head1 EXAMPLE
 
 Given an OpenSRF configuration file named F<opensrf_core.xml> with the
@@ -174,12 +173,29 @@ section of C<$config_obj>; for example:
 
 =head1 NOTES
 
-For compatibility with a previous version of OpenSRF configuration
-files, the F</config/opensrf/> section has a hardcoded name of
-B<bootstrap>. However, future iterations of this module may extend the
-ability of the module to parse the entire OpenSRF configuration file
-and provide sections named after the sibling elements of
-C</config/opensrf>.
+For compatibility with previous versions of the OpenSRF configuration
+files, the C<load()> method by default loads the C</config/opensrf>
+section with the hardcoded name of B<bootstrap>.
+
+However, it is possible to load child elements of C<< <config> >> other
+than C<< <opensrf> >> by supplying a C<base_path> argument which specifies
+the node you wish to begin loading from (in XPath notation). Doing so
+will also replace the hardcoded C<bootstrap> name with the node name of
+the last member of the given path.  For example:
+
+  my $config_obj = OpenSRF::Utils::Config->load(
+      config_file => '/config/file.cnf'
+      base_path => '/config/shared'
+  );
+
+  my $attrs_href = $config_obj->shared();
+
+While it may be possible to load the entire file in this fashion (by
+specifying an empty C<base_path>), doing so will break compatibility with
+existing code which expects to find a C<bootstrap> member. Future
+iterations of this module may extend its ability to parse the entire
+OpenSRF configuration file in one pass while providing multiple base
+sections named after the sibling elements of C</config/opensrf>.
 
 Hashrefs of sections can be returned by calling a method of the object
 of the same name as the section.  They can be set by passing a hashref
@@ -242,7 +258,11 @@ sub _load {
        delete $$self{config_file};
        return undef unless ($self->FILE);
 
-       $self->load_config();
+       my %load_args;
+       if (exists $$self{base_path}) { # blank != non-existent for this setting
+               $load_args{base_path} = $$self{base_path};
+       }
+       $self->load_config(%load_args);
        $self->load_env();
        $self->mangle_dirs();
        $self->mangle_logs();
@@ -250,6 +270,7 @@ sub _load {
        $OpenSRF::Utils::ConfigCache = $self unless $self->nocache;
        delete $$self{nocache};
        delete $$self{force};
+       delete $$self{base_path};
        return $self;
 }
 
@@ -315,6 +336,7 @@ sub mangle_dirs {
 sub load_config {
        my $self = shift;
        my $parser = XML::LibXML->new();
+       my %args = @_;
 
        # Hash of config values
        my %bootstrap;
@@ -327,9 +349,16 @@ sub load_config {
                die "Could not open ".$self->FILE.": $!\n";
        }
 
+       # For backwards compatibility, we default to /config/opensrf
+       my $base_path;
+       if (exists $args{base_path}) { # allow for empty to import entire file
+               $base_path = $args{base_path};
+       } else {
+               $base_path = '/config/opensrf';
+       }
        # Return an XML::LibXML::NodeList object matching all child elements
-       # of <config><opensrf>...
-       my $osrf_cfg = $config->findnodes('/config/opensrf/child::*');
+       # of $base_path...
+       my $osrf_cfg = $config->findnodes("$base_path/child::*");
 
        # Iterate through the nodes to pull out key=>value pairs of config settings
        foreach my $node ($osrf_cfg->get_nodelist()) {
@@ -354,6 +383,13 @@ sub load_config {
        }
 
        my $section = $self->section_pkg->new(\%bootstrap);
+       # if the Config was loaded with a 'base_path' option, overwrite the
+       # hardcoded 'bootstrap' name with something more reasonable
+       if (exists $$self{base_path}) { # blank != non-existent for this setting
+               # name root node to reflect last member of base_path, or default to root
+               my $root = (split('/', $$self{base_path}))[-1] || 'root';
+               $section->__id($root);
+       }
        my $sub_name = $section->SECTION;
        $self->_sub_builder($sub_name);
        $self->$sub_name($section);