On 07/21/2018 02:57 PM, John Ferlan wrote: > > > 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@xxxxxxxxxx> >> --- >> 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? Because "sysinfotest" is not one of the array ("open", "fopen", "access", ...) The idea is to have two sets of rules: /some/path: program /some/path: open: program The former allows just ANY access to /some/path for program "program", i.e. it can open, fopen, access, stat, lstat, connect to it. The latter allows only one action over /some/path. For instance open("/some/path") is allowed but stat("/some/path") isn't. To complicate things a bit more, if I wanted to allow /some/path for every program, instead of having one rule per each program I can only have: /some/path /some/path: open The former allows ANY access to /some/path for ANY program, the latter allows open("/some/path") for ANY program but disallows other types of access. Joining types of access is not supported yet. For instance: /some/path: open stat has to be written as: /some/path: open /some/path: stat And when I say allowed what I really mean is: reported by 'make check-access'. True, we are missing a lot of rules there. But I have an idea how to fill them out ;-) Michal -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list