[libvirt] [PATCH v2 4/5] check-file-access: Allow specifying action

[John Ferlan]

On 07/12/2018 03:37 AM, Michal Privoznik wrote:
> The check-file-access.pl script is used to match access list
> generated by virtestmock against whitelisted rules stored in
> file_access_whitelist.txt. So far the rules are in form:
> 
>   $path: $progname: $testname
> 
> This is not sufficient because the rule does not take into
> account 'action' that caused $path to appear in the list of
> accessed files. After this commit the rule can be in new form:
> 
>   $path: $action: $progname: $testname
> 
> where $action is one from ("open", "fopen", "access", "stat",
> "lstat", "connect"). This way the white list can be fine tuned to
> allow say access() but not connect().
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  tests/check-file-access.pl      | 32 +++++++++++++++++++++++++++-----
>  tests/file_access_whitelist.txt | 15 ++++++++++-----
>  2 files changed, 37 insertions(+), 10 deletions(-)
> 

Perl scripts, not my area of expertise...  Even worse, regexes.

Still I am unclear about this particular one because you stuffed $action
in front of something that already existed and it makes me wonder how
that affects existing files. [1]


> diff --git a/tests/check-file-access.pl b/tests/check-file-access.pl
> index 977a2bc533..ea0b7a18a2 100755
> --- a/tests/check-file-access.pl
> +++ b/tests/check-file-access.pl
> @@ -27,18 +27,21 @@ use warnings;
>  my $access_file = "test_file_access.txt";
>  my $whitelist_file = "file_access_whitelist.txt";
>  
> +my @known_actions = ("open", "fopen", "access", "stat", "lstat", "connect");
> +
>  my @files;
>  my @whitelist;
>  
>  open FILE, "<", $access_file or die "Unable to open $access_file: $!";
>  while (<FILE>) {
>      chomp;
> -    if (/^(\S*):\s*(\S*)(\s*:\s*(.*))?$/) {
> +    if (/^(\S*):\s*(\S*):\s*(\S*)(\s*:\s*(.*))?$/) {
>          my %rec;
>          ${rec}{path} = $1;
> -        ${rec}{progname} = $2;
> -        if (defined $4) {
> -            ${rec}{testname} = $4;
> +        ${rec}{action} = $2;
> +        ${rec}{progname} = $3;
> +        if (defined $5) {
> +            ${rec}{testname} = $5;
>          }
>          push (@files, \%rec);
>      } else {
> @@ -52,7 +55,21 @@ while (<FILE>) {
>      chomp;
>      if (/^\s*#.*$/) {
>          # comment
> +    } elsif (/^(\S*):\s*(\S*)(:\s*(\S*)(\s*:\s*(.*))?)?$/ and
> +            grep /^$2$/, @known_actions) {
> +        # $path: $action: $progname: $testname
> +        my %rec;
> +        ${rec}{path} = $1;
> +        ${rec}{action} = $3;
> +        if (defined $4) {
> +            ${rec}{progname} = $4;
> +        }
> +        if (defined $6) {
> +            ${rec}{testname} = $6;
> +        }
> +        push (@whitelist, \%rec);
>      } elsif (/^(\S*)(:\s*(\S*)(\s*:\s*(.*))?)?$/) {
> +        # $path: $progname: $testname
>          my %rec;
>          ${rec}{path} = $1;
>          if (defined $3) {
> @@ -79,6 +96,11 @@ for my $file (@files) {
>              next;
>          }
>  
> +        if (defined %${rule}{action} and
> +            not %${file}{action} =~ m/^$rule->{action}$/) {
> +            next;
> +        }
> +
>          if (defined %${rule}{progname} and
>              not %${file}{progname} =~ m/^$rule->{progname}$/) {
>              next;
> @@ -95,7 +117,7 @@ for my $file (@files) {
>  
>      if (not $match) {
>          $error = 1;
> -        print "$file->{path}: $file->{progname}";
> +        print "$file->{path}: $file->{action}: $file->{progname}";
>          print ": $file->{testname}" if defined %${file}{testname};
>          print "\n";
>      }
> diff --git a/tests/file_access_whitelist.txt b/tests/file_access_whitelist.txt
> index 850b28506e..3fb318cbab 100644
> --- a/tests/file_access_whitelist.txt
> +++ b/tests/file_access_whitelist.txt
> @@ -1,14 +1,17 @@
>  # This is a whitelist that allows accesses to files not in our
>  # build directory nor source directory. The records are in the
> -# following format:
> +# following formats:
>  #
>  #  $path: $progname: $testname
> +#  $path: $action: $progname: $testname
>  #
> -# All these three are evaluated as perl RE. So to allow /dev/sda
> -# and /dev/sdb, you can just '/dev/sd[a-b]', or to allow
> +# All these variables are evaluated as perl RE. So to allow
> +# /dev/sda and /dev/sdb, you can just '/dev/sd[a-b]', or to allow
>  # /proc/$pid/status you can '/proc/\d+/status' and so on.
> -# Moreover, $progname and $testname can be empty, in which which
> -# case $path is allowed for all tests.
> +# Moreover, $action, $progname and $testname can be empty, in which
> +# which case $path is allowed for all tests. However, $action (if
> +# specified) must be one of "open", "fopen", "access", "stat",
> +# "lstat", "connect".
>  
>  /bin/cat: sysinfotest
>  /bin/dirname: sysinfotest: x86 sysinfo

[1] For example, why wouldn't sysinfotest in this case relate to $action
instead of $progname?

Are things read backwards - I just don't feel comfortable enough about
this code to review in much depth. Since patch 5 is based upon this
change, I'll also stop here.

John

BTW: I have to say the output in patch5 at least explains something else
I saw as a result of this series that I couldn't quite explain. I recall
seeing the message and wondering, WTF it was trying to tell me.


> @@ -19,5 +22,7 @@
>  /etc/hosts
>  /proc/\d+/status
>  


> +/etc/passwd: fopen
> +
>  # This is just a dummy example, DO NOT USE IT LIKE THAT!
>  .*: nonexistent-test-touching-everything
>