Re: [libvirt] [PATCH v2 4/5] check-file-access: Allow specifying action
On 07/23/2018 04:44 AM, Michal Prívozník wrote:
> 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
>>> ---
>>> 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 () {
>>> 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",
Re: [libvirt] [PATCH v2 4/5] check-file-access: Allow specifying action
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
>> ---
>> 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 () {
>> 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
>
> [1] For example, why wouldn't sysinfotest in this case
Re: [libvirt] [PATCH v2 4/5] check-file-access: Allow specifying action
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
> ---
> 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 () {
> 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
[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,
[libvirt] [PATCH v2 4/5] check-file-access: Allow specifying action
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
