Re: [libvirt] [PATCH v3 2/3] check-file-access: Allow specifying action

2018-08-14 Thread John Ferlan



On 07/27/2018 11:24 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 
> ---
>  tests/check-file-access.pl  | 32 +++-
>  tests/file_access_whitelist.txt | 15 ++-
>  2 files changed, 37 insertions(+), 10 deletions(-)
> 

I think based on the previous time through this and the explanation
provided afterwards I am comfortable with the changes. Still it would be
nice perhaps to alter the comments in file_access_whitelist.txt in order
to describe the various settings like you replied here:

https://www.redhat.com/archives/libvir-list/2018-July/msg01434.html

starting with "The idea is to have two sets of rules:" and copying
enough of that in order to provide an example in the comments so that
someone who really didn't have the desire or cycles to read the perl
script could actually write a reasonable rule.

Knowing "how" or "when" to use may be a good idea. After patch 1 there's
no longer an example in the qemuxml2argvtest output.

Consider it a weak because my perl scripting and regex knowledge isn't
the best...

Reviewed-by: John Ferlan 

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


[libvirt] [PATCH v3 2/3] check-file-access: Allow specifying action

2018-07-27 Thread Michal Privoznik
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 
---
 tests/check-file-access.pl  | 32 +++-
 tests/file_access_whitelist.txt | 15 ++-
 2 files changed, 37 insertions(+), 10 deletions(-)

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 () {
 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 () {
 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
@@ -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
-- 
2.16.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list