On 2/14/20 3:56 AM, Richard Haines wrote:
Use the filesystem type that the selinux-testsuite is running from to be
used for tests/filesystem and tests/fs_filesystem.
If testing locally the -f <fs_type> can be used to test a specific type.
These are the tested and supported filesystem types: ext2, ext3, ext4, xfs,
btrfs, hfsplus, reiserfs, nfs4. If not in this list, tests are skipped.
Same comment as for the cover letter: ext4, xfs, nfs4 are the main ones
of interest. If you want to also allow for running the tests on ext[23]
and btrfs that is fine but I wouldn't bother with hfsplus or reiserfs.
I don't think you actually need a whitelist at all though. If someone
runs the test on an unsupported filesystem and it fails, that's ok -
that is correctly informing them that their filesystem doesn't support
that aspect of SELinux functionality. Why bother whitelisting and
skipping tests in that situation?
I don't know if we might want to also include a test of context= mount
functionality for a filesystem type that doesn't support file security
labeling natively, e.g. vfat? That's a common use case for context=
mounts for removable media.
Signed-off-by: Richard Haines <richard_c_haines@xxxxxxxxxxxxxx>
---
diff --git a/policy/test_filesystem.te b/policy/test_filesystem.te
index 09f9d4a..fd928de 100644
--- a/policy/test_filesystem.te
+++ b/policy/test_filesystem.te
allow test_filesystem_t test_filesystem_filetranscon_t:file { create getattr open write relabelfrom };
dontaudit unconfined_t test_filesystem_filetranscon_t:file { getattr read };
+#
+############## Additional reiserfs rules ########################
Comment seems suspect (reiserfs above versus nfs below).
+#
+gen_require(`
+ type nfs_t;
+')
+allow test_filesystem_no_getattr_t unlabeled_t:dir { search };
Why unlabeled_t? That seems like a bug. Don't hide bugs in the test
policy or code; we want them exposed as failures.
+allow test_filesystem_no_getattr_t nfs_t:filesystem { associate };
Could allow for all of your test domains via a single rule on the
"filesystemdomain" attribute? Kind of weird using a domain type in a
rootcontext= mount option but whatever.
+#
+############## Additional hfsplus rules ########################
Drop hfsplus, maybe switch to vfat testing of just context= mounts?
+#
+############### Additional NFS rules ###############
+#
+##### NFS mount option: rootcontext=system_u:object_r:test_filesystem_file_t:s0
+# Note that defcontext is not supported by nfs
+allow_map(test_filesystem_t, test_filesystem_file_t, file)
+allow test_filesystem_t test_filesystem_file_t:dir { mounton };
+allow test_filesystem_t test_filesystem_file_t:file { entrypoint execute read };
Why are you executing from the mount?
+allow test_filesystem_t test_filesystem_file_t:filesystem { mount getattr remount relabelfrom relabelto unmount };
+
+# Test file:
+allow test_filesystem_t test_file_t:file { create relabelfrom write };
+
+gen_require(`
+ type user_home_t;
+')
+allow test_no_setfscreatecon_t user_home_t:dir { search };
+allow test_setfscreatecon_t user_home_t:dir { search };
For these and all subsequent references to user_home_t, use a single
rule on an attribute and try to use an interface to avoid assuming it
must be user_home_t (versus admin_home_t or whatever). Maybe
files_list_home(filesystemdomain) or
userdom_search_user_home_content(filesystemdomain)?
Throughout, try to rewrite to use attributes to reduce identical rules.
diff --git a/tests/filesystem/Filesystem.pm b/tests/filesystem/Filesystem.pm
index a08570a..20b01af 100644
--- a/tests/filesystem/Filesystem.pm
+++ b/tests/filesystem/Filesystem.pm
@@ -111,17 +111,30 @@ sub attach_dev {
sub make_fs {
my ( $mk_type, $mk_dev, $mk_dir ) = @_;
+
+ if ( $mk_type eq "btrfs" or $mk_type eq "reiserfs" ) {
+ $count = "count=27904";
+ }
Why does btrfs or reiserfs need a weird count value? Why that
particular value? In any event, I wouldn't go out of your way to
support either one. If there is some sane value that we can use for all
filesystem types, let's use that; otherwise just let it break on those
filesystems.
+ $opt = " ";
+ if ( $mk_type eq "reiserfs" ) {
+ $opt = "-q"; # Otherwise asks to proceed
+ }
No need to support reiserfs specially IMHO.
diff --git a/tests/filesystem/test b/tests/filesystem/test
index 78faf72..b8d14ed 100755
--- a/tests/filesystem/test
+++ b/tests/filesystem/test
@@ -12,19 +12,82 @@ BEGIN {
<snip>
+ # If NFS specified, exit as cannot run locally CHECK IF NFS RUNNING ???
+ if ( $fs_type eq "nfs4" or $fs_type eq "nfs" ) {
+ plan skip_all => "Skip tests as running $fs_type locally not supported";
+ }
Super-confused here. NFS supports running locally as evidenced by my
tools/nfs.sh script and README.md instructions. And you said you were
going to support it on the cover and patch description. Why skip out
here? And why do you have to test for both nfs4 and nfs? My logic for
skipping certain tests on nfs only needed to check for nfs based on
output of stat -f --print %T $basedir.
+ if ( $fs_type ne "ext2"
+ and $fs_type ne "ext3"
+ and $fs_type ne "ext4"
+ and $fs_type ne "xfs"
+ and $fs_type ne "btrfs"
+ and $fs_type ne "hfsplus"
+ and $fs_type ne "reiserfs"
+ and $fs_type ne "nfs4"
+ and $fs_type ne "nfs" )
+ {
+ plan skip_all => "Skip tests as $fs_type is not supported";
+ }
IMHO no need for this whitelist or logic at all. Just run the tests on
whatever filesystem we have and if it fails, it fails. That's ok.
+ print "Testing filesystem type: $fs_type\n";
+
+ if ( $fs_type eq "nfs4" or $fs_type eq "nfs" ) {
+ system("$basedir/test-nfs.pl $v -f $fs_type");
+ exit 0;
Hmmm...if test-nfs.pl fails the error won't get propagated up? And do
we really need a separate test script for it?
+ }
+
+ # Note: ext2, ext3, ext4, f2fs, reiserfs and jfs call dquot_quota_on();
+ # therefore could check qouta permissions
+ if (
+ $fs_type eq "xfs" # Requires xfs_quota(8)
+ or $fs_type eq "btrfs" # Requires btrfs_quota(8)
+ or $fs_type eq "hfsplus" # Does not support quotas
+ or $fs_type eq "reiserfs" # Has internal quota.
+ or $fs_type eq "nfs4" # Does not support quotas
+ or $fs_type eq "nfs"
+ )
+ {
+ $test_count = 54;
+ $quota_checks = 0;
+ }
+ else {
+ $test_count = 68;
+ }
Do the quota tests pass if you install xfs_quota? If so, just note it
as a dependency when running on a xfs filesystem in the README.md and
run the tests. On nfs4/nfs, skip the tests like we do for other
functionality not supported on nfs. The rest I'd leave out and just
allow to fail until such a time as someone cares.
+
+ # These do not support defcontext tests
+ if ( $fs_type eq "reiserfs"
+ or $fs_type eq "hfsplus"
+ or $fs_type eq "nfs4"
+ or $fs_type eq "nfs" )
+ {
+ $test_count -= 6;
Skip on nfs/nfs4; otherwise we don't care.
@@ -129,7 +197,12 @@ $result =
system(
"runcon -t test_filesystem_t $basedir/create_file_change_context -t test_filesystem_filecon_t -f $private_path/mp1/test_file $v"
);
-ok( $result eq 0 );
+if ( $fs_type eq "reiserfs" or $fs_type eq "hfsplus" ) {
+ ok( $result >> 8 eq 95 ); # EOPNOTSUPP
+}
+else {
+ ok( $result eq 0 );
+}
Drop - we don't care about reiserfs or hfsplus SELinux support; let it
fail and if someone cares they should implement the actual support not
just skip it.
@@ -221,7 +293,7 @@ mk_mntpoint_1($private_path);
( $dev, $device_count ) = get_loop_dev( \@device_list, $device_count );
make_fs( $fs_type, $dev, $basedir );
$opts_no_relabelfrom =
- "defcontext=system_u:object_r:test_filesystem_sb_relabel_no_relabelfrom_t:s0";
+"rootcontext=system_u:object_r:test_filesystem_sb_relabel_no_relabelfrom_t:s0";
Does this mean we won't exercise the check even on filesystem types that
support it? That doesn't seem desirable. Optimally we'd test it for both.
@@ -312,7 +384,12 @@ $result =
system(
"runcon -t test_filesystem_may_create_no_associate_t $basedir/create_file_change_context -t unconfined_t -f $basedir/mntpoint/mp1/test_file $v 2>&1"
);
-ok( $result >> 8 eq 13 );
+if ( $fs_type eq "reiserfs" or $fs_type eq "hfsplus" ) {
+ ok( $result >> 8 eq 95 ); # EOPNOTSUPP
+}
Don't care about testing reiserfs or hfsplus.
diff --git a/tests/filesystem/test-nfs.pl b/tests/filesystem/test-nfs.pl
new file mode 100755
index 0000000..d6a931d
--- /dev/null
+++ b/tests/filesystem/test-nfs.pl
<snip>
+############### Test setfscreatecon(3) ##########################
+system("mkdir -p $basedir/mntpoint 2>/dev/null");
+
+print "Test setfscreatecon(3)\n";
+$result = system
+"runcon -t test_setfscreatecon_t $basedir/fs_relabel $v -b $basedir/mntpoint -t test_setfscreatecon_newcon_t";
+ok( $result eq 0 );
+
+$result = system
+"runcon -t test_no_setfscreatecon_t $basedir/fs_relabel $v -b $basedir/mntpoint -t test_setfscreatecon_newcon_t 2>&1";
+ok( $result >> 8 eq 13 );
No, we don't want to replicate the tests in another script that has to
be maintained separately. The goal is to exercise the same test code on
whatever filesystem we have. So you could take parts of this new script
back into test to set up the mount, but then we should proceed and run
as many of the tests in the test script as are feasible on NFS.
diff --git a/tools/nfs.sh b/tools/nfs.sh
index 314f898..fb235dc 100755
--- a/tools/nfs.sh
+++ b/tools/nfs.sh
@@ -1,4 +1,16 @@
#!/bin/sh -e
+
+# If 'make test' fails, close cleanly
+function err_exit() {
+ popd
+ umount /mnt/selinux-testsuite
+ exportfs -u localhost:$MOUNT
+ rmdir /mnt/selinux-testsuite
+ systemctl stop nfs-server
+}
+
+trap 'err_exit' EXIT
+
That's a nice cleanup regardless of the rest of this patch; feel free to
separate it out and submit it.
MOUNT=`stat --print %m .`
TESTDIR=`pwd`
systemctl start nfs-server
@@ -7,6 +19,10 @@ systemctl start nfs-server
exportfs -orw,no_root_squash,security_label localhost:$MOUNT
mkdir -p /mnt/selinux-testsuite
mount -t nfs -o vers=4.2 localhost:$TESTDIR /mnt/selinux-testsuite
+# These may be used for tests/filesystem only at present as there is
+# a bug in fsconfig(2), therefore tests/fs_filesystem will fail:
+# mount -t nfs -o vers=4.2,rootcontext=system_u:object_r:test_filesystem_file_t:s0 localhost:$TESTDIR /mnt/selinux-testsuite
+# mount -t nfs -o vers=4.2,fscontext=system_u:object_r:test_filesystem_file_t:s0 localhost:$TESTDIR /mnt/selinux-testsuite
pushd /mnt/selinux-testsuite
make test
popd
No, we should leave those tests failing until they are fixed - it is a
real bug.
