[PATCH v2 0/6] export/fuse: Allow other users access to the export

[PATCH v2 0/6] export/fuse: Allow other users access to the export

[Max Reitz]

Hi,

The v1 cover letter is here:
https://lists.nongnu.org/archive/html/qemu-block/2021-06/msg00730.html

In v2, I changed the following:
- default_permissions is now passed always.  This is the right thing to
  do regardless of whether allow_other is active or not.

- allow_other is no longer a bool, but an off/on/auto enum.  `auto` is
  the default, in which case we will try to mount the export with
  allow_other first, and then fall back to mounting it without.

- Changing the file mode is now possible even without allow_other
  (because default_permissions is always active now), but only for the
  user/owner.  Giving the group or others any permissions only makes
  sense with allow_other, the same applies to changing the UID or GID.
  Giving a read-only export +w makes no sense and hence yields an EROFS
  error now.

- I decided just testing some default_permission quirks is boring.  So
  the new fuse-allow-other iotest does rely on `sudo -n -u nobody`
  working now, and actually tests what allow_other is supposed to do.
  (Also, it is skipped if allow_other does not work.)


git-backport-diff against v1:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/6:[down] 'export/fuse: Pass default_permissions for mount'
002/6:[0089] [FC] 'export/fuse: Add allow-other option'
003/6:[----] [--] 'export/fuse: Give SET_ATTR_SIZE its own branch'
004/6:[0039] [FC] 'export/fuse: Let permissions be adjustable'
005/6:[down] 'iotests/308: Test +w on read-only FUSE exports'
006/6:[down] 'iotests/fuse-allow-other: Test allow-other'


Max Reitz (6):
  export/fuse: Pass default_permissions for mount
  export/fuse: Add allow-other option
  export/fuse: Give SET_ATTR_SIZE its own branch
  export/fuse: Let permissions be adjustable
  iotests/308: Test +w on read-only FUSE exports
  iotests/fuse-allow-other: Test allow-other

 qapi/block-export.json                        |  33 +++-
 block/export/fuse.c                           | 121 +++++++++---
 tests/qemu-iotests/308                        |  20 +-
 tests/qemu-iotests/308.out                    |   6 +-
 tests/qemu-iotests/common.rc                  |   6 +-
 tests/qemu-iotests/tests/fuse-allow-other     | 175 ++++++++++++++++++
 tests/qemu-iotests/tests/fuse-allow-other.out |  88 +++++++++
 7 files changed, 421 insertions(+), 28 deletions(-)
 create mode 100755 tests/qemu-iotests/tests/fuse-allow-other
 create mode 100644 tests/qemu-iotests/tests/fuse-allow-other.out

-- 
2.31.1

[PATCH v2 1/6] export/fuse: Pass default_permissions for mount

[Max Reitz]

We do not do any permission checks in fuse_open(), so let the kernel do
them.  We already let fuse_getattr() report the proper UNIX permissions,
so this should work the way we want.

This causes a change in 308's reference output, because now opening a
non-writable export with O_RDWR fails already, instead of only actually
attempting to write to it.  (That is an improvement.)

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/export/fuse.c        | 8 ++++++--
 tests/qemu-iotests/308     | 3 ++-
 tests/qemu-iotests/308.out | 2 +-
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/block/export/fuse.c b/block/export/fuse.c
index 38f74c94da..d0b88e8f80 100644
--- a/block/export/fuse.c
+++ b/block/export/fuse.c
@@ -153,8 +153,12 @@ static int setup_fuse_export(FuseExport *exp, const char *mountpoint,
     struct fuse_args fuse_args;
     int ret;
 
-    /* Needs to match what fuse_init() sets.  Only max_read must be supplied. */
-    mount_opts = g_strdup_printf("max_read=%zu", FUSE_MAX_BOUNCE_BYTES);
+    /*
+     * max_read needs to match what fuse_init() sets.
+     * max_write need not be supplied.
+     */
+    mount_opts = g_strdup_printf("max_read=%zu,default_permissions",
+                                 FUSE_MAX_BOUNCE_BYTES);
 
     fuse_argv[0] = ""; /* Dummy program name */
     fuse_argv[1] = "-o";
diff --git a/tests/qemu-iotests/308 b/tests/qemu-iotests/308
index f122065d0f..11c28a75f2 100755
--- a/tests/qemu-iotests/308
+++ b/tests/qemu-iotests/308
@@ -215,7 +215,8 @@ echo '=== Writable export ==='
 fuse_export_add 'export-mp' "'mountpoint': '$EXT_MP', 'writable': true"
 
 # Check that writing to the read-only export fails
-$QEMU_IO -f raw -c 'write -P 42 1M 64k' "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -f raw -c 'write -P 42 1M 64k' "$TEST_IMG" 2>&1 \
+    | _filter_qemu_io | _filter_testdir | _filter_imgfmt
 
 # But here it should work
 $QEMU_IO -f raw -c 'write -P 42 1M 64k' "$EXT_MP" | _filter_qemu_io
diff --git a/tests/qemu-iotests/308.out b/tests/qemu-iotests/308.out
index 466e7e0267..0e9420645f 100644
--- a/tests/qemu-iotests/308.out
+++ b/tests/qemu-iotests/308.out
@@ -91,7 +91,7 @@ virtual size: 0 B (0 bytes)
               'mountpoint': 'TEST_DIR/t.IMGFMT.fuse', 'writable': true
           } }
 {"return": {}}
-write failed: Permission denied
+qemu-io: can't open device TEST_DIR/t.IMGFMT: Could not open 'TEST_DIR/t.IMGFMT': Permission denied
 wrote 65536/65536 bytes at offset 1048576
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 wrote 65536/65536 bytes at offset 1048576
-- 
2.31.1

[PATCH v2 2/6] export/fuse: Add allow-other option

[Max Reitz]

Without the allow_other mount option, no user (not even root) but the
one who started qemu/the storage daemon can access the export.  Allow
users to configure the export such that such accesses are possible.

While allow_other is probably what users want, we cannot make it an
unconditional default, because passing it is only possible (for non-root
users) if the global fuse.conf configuration file allows it.  Thus, the
default is an 'auto' mode, in which we first try with allow_other, and
then fall back to without.

FuseExport.allow_other reports whether allow_other was actually used as
a mount option or not.  Currently, this information is not used, but a
future patch will let this field decide whether e.g. an export's UID and
GID can be changed through chmod.

One notable thing about 'auto' mode is that libfuse may print error
messages directly to stderr, and so may fusermount (which it executes).
Our export code cannot really filter or hide them.  Therefore, if 'auto'
fails its first attempt and has to fall back, fusermount will print an
error message that mounting with allow_other failed.

This behavior necessitates a change to iotest 308, namely we need to
filter out this error message (because if the first attempt at mounting
with allow_other succeeds, there will be no such message).

Furthermore, common.rc's _make_test_img should use allow-other=off for
FUSE exports, because iotests generally do not need to access images
from other users, so allow-other=on or allow-other=auto have no
advantage.  OTOH, allow-other=on will not work on systems where
user_allow_other is disabled, and with allow-other=auto, we get said
error message that we would need to filter out again.  Just disabling
allow-other is simplest.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qapi/block-export.json       | 33 ++++++++++++++++++++++++++++++++-
 block/export/fuse.c          | 28 +++++++++++++++++++++++-----
 tests/qemu-iotests/308       |  6 +++++-
 tests/qemu-iotests/common.rc |  6 +++++-
 4 files changed, 65 insertions(+), 8 deletions(-)

diff --git a/qapi/block-export.json b/qapi/block-export.json
index e819e70cac..0ed63442a8 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -120,6 +120,23 @@
 	    '*logical-block-size': 'size',
             '*num-queues': 'uint16'} }
 
+##
+# @FuseExportAllowOther:
+#
+# Possible allow_other modes for FUSE exports.
+#
+# @off: Do not pass allow_other as a mount option.
+#
+# @on: Pass allow_other as a mount option.
+#
+# @auto: Try mounting with allow_other first, and if that fails, retry
+#        without allow_other.
+#
+# Since: 6.1
+##
+{ 'enum': 'FuseExportAllowOther',
+  'data': ['off', 'on', 'auto'] }
+
 ##
 # @BlockExportOptionsFuse:
 #
@@ -132,11 +149,25 @@
 # @growable: Whether writes beyond the EOF should grow the block node
 #            accordingly. (default: false)
 #
+# @allow-other: If this is off, only qemu's user is allowed access to
+#               this export.  That cannot be changed even with chmod or
+#               chown.
+#               Enabling this option will allow other users access to
+#               the export with the FUSE mount option "allow_other".
+#               Note that using allow_other as a non-root user requires
+#               user_allow_other to be enabled in the global fuse.conf
+#               configuration file.
+#               In auto mode (the default), the FUSE export driver will
+#               first attempt to mount the export with allow_other, and
+#               if that fails, try again without.
+#               (since 6.1; default: auto)
+#
 # Since: 6.0
 ##
 { 'struct': 'BlockExportOptionsFuse',
   'data': { 'mountpoint': 'str',
-            '*growable': 'bool' },
+            '*growable': 'bool',
+            '*allow-other': 'FuseExportAllowOther' },
   'if': 'defined(CONFIG_FUSE)' }
 
 ##
diff --git a/block/export/fuse.c b/block/export/fuse.c
index d0b88e8f80..4068250241 100644
--- a/block/export/fuse.c
+++ b/block/export/fuse.c
@@ -46,6 +46,8 @@ typedef struct FuseExport {
     char *mountpoint;
     bool writable;
     bool growable;
+    /* Whether allow_other was used as a mount option or not */
+    bool allow_other;
 } FuseExport;
 
 static GHashTable *exports;
@@ -57,7 +59,7 @@ static void fuse_export_delete(BlockExport *exp);
 static void init_exports_table(void);
 
 static int setup_fuse_export(FuseExport *exp, const char *mountpoint,
-                             Error **errp);
+                             bool allow_other, Error **errp);
 static void read_from_fuse_export(void *opaque);
 
 static bool is_regular_file(const char *path, Error **errp);
@@ -118,7 +120,22 @@ static int fuse_export_create(BlockExport *blk_exp,
     exp->writable = blk_exp_args->writable;
     exp->growable = args->growable;
 
-    ret = setup_fuse_export(exp, args->mountpoint, errp);
+    /* set default */
+    if (!args->has_allow_other) {
+        args->allow_other = FUSE_EXPORT_ALLOW_OTHER_AUTO;
+    }
+
+    if (args->allow_other == FUSE_EXPORT_ALLOW_OTHER_AUTO) {
+        /* Ignore errors on our first attempt */
+        ret = setup_fuse_export(exp, args->mountpoint, true, NULL);
+        exp->allow_other = ret == 0;
+        if (ret < 0) {
+            ret = setup_fuse_export(exp, args->mountpoint, false, errp);
+        }
+    } else {
+        exp->allow_other = args->allow_other == FUSE_EXPORT_ALLOW_OTHER_ON;
+        ret = setup_fuse_export(exp, args->mountpoint, exp->allow_other, errp);
+    }
     if (ret < 0) {
         goto fail;
     }
@@ -146,7 +163,7 @@ static void init_exports_table(void)
  * Create exp->fuse_session and mount it.
  */
 static int setup_fuse_export(FuseExport *exp, const char *mountpoint,
-                             Error **errp)
+                             bool allow_other, Error **errp)
 {
     const char *fuse_argv[4];
     char *mount_opts;
@@ -157,8 +174,9 @@ static int setup_fuse_export(FuseExport *exp, const char *mountpoint,
      * max_read needs to match what fuse_init() sets.
      * max_write need not be supplied.
      */
-    mount_opts = g_strdup_printf("max_read=%zu,default_permissions",
-                                 FUSE_MAX_BOUNCE_BYTES);
+    mount_opts = g_strdup_printf("max_read=%zu,default_permissions%s",
+                                 FUSE_MAX_BOUNCE_BYTES,
+                                 allow_other ? ",allow_other" : "");
 
     fuse_argv[0] = ""; /* Dummy program name */
     fuse_argv[1] = "-o";
diff --git a/tests/qemu-iotests/308 b/tests/qemu-iotests/308
index 11c28a75f2..d13a9a969c 100755
--- a/tests/qemu-iotests/308
+++ b/tests/qemu-iotests/308
@@ -58,6 +58,9 @@ _supported_os Linux # We need /dev/urandom
 # $4: Node to export (defaults to 'node-format')
 fuse_export_add()
 {
+    # The grep -v is a filter for errors when /etc/fuse.conf does not contain
+    # user_allow_other.  (The error is benign, but it is printed by fusermount
+    # on the first mount attempt, so our export code cannot hide it.)
     _send_qemu_cmd $QEMU_HANDLE \
         "{'execute': 'block-export-add',
           'arguments': {
@@ -67,7 +70,8 @@ fuse_export_add()
               $2
           } }" \
         "${3:-return}" \
-        | _filter_imgfmt
+        | _filter_imgfmt \
+        | grep -v 'option allow_other only allowed if'
 }
 
 # $1: Export ID
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index cbbf6d7c7f..609d82de89 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -512,9 +512,13 @@ _make_test_img()
         # Usually, users would export formatted nodes.  But we present fuse as a
         # protocol-level driver here, so we have to leave the format to the
         # client.
+        # Switch off allow-other, because in general we do not need it for
+        # iotests.  The default allow-other=auto has the downside of printing a
+        # fusermount error on its first attempt if allow_other is not
+        # permissible, which we would need to filter.
         QSD_NEED_PID=y $QSD \
               --blockdev file,node-name=export-node,filename=$img_name,discard=unmap \
-              --export fuse,id=fuse-export,node-name=export-node,mountpoint="$export_mp",writable=on,growable=on \
+              --export fuse,id=fuse-export,node-name=export-node,mountpoint="$export_mp",writable=on,growable=on,allow-other=off \
               &
 
         pidfile="$QEMU_TEST_DIR/qemu-storage-daemon.pid"
-- 
2.31.1

[PATCH v2 3/6] export/fuse: Give SET_ATTR_SIZE its own branch

[Max Reitz]

In order to support changing other attributes than the file size in
fuse_setattr(), we have to give each its own independent branch.  This
also applies to the only attribute we do support right now.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 block/export/fuse.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/block/export/fuse.c b/block/export/fuse.c
index 4068250241..26ad644cd7 100644
--- a/block/export/fuse.c
+++ b/block/export/fuse.c
@@ -417,20 +417,22 @@ static void fuse_setattr(fuse_req_t req, fuse_ino_t inode, struct stat *statbuf,
     FuseExport *exp = fuse_req_userdata(req);
     int ret;
 
-    if (!exp->writable) {
-        fuse_reply_err(req, EACCES);
-        return;
-    }
-
     if (to_set & ~FUSE_SET_ATTR_SIZE) {
         fuse_reply_err(req, ENOTSUP);
         return;
     }
 
-    ret = fuse_do_truncate(exp, statbuf->st_size, true, PREALLOC_MODE_OFF);
-    if (ret < 0) {
-        fuse_reply_err(req, -ret);
-        return;
+    if (to_set & FUSE_SET_ATTR_SIZE) {
+        if (!exp->writable) {
+            fuse_reply_err(req, EACCES);
+            return;
+        }
+
+        ret = fuse_do_truncate(exp, statbuf->st_size, true, PREALLOC_MODE_OFF);
+        if (ret < 0) {
+            fuse_reply_err(req, -ret);
+            return;
+        }
     }
 
     fuse_getattr(req, inode, fi);
-- 
2.31.1

[PATCH v2 4/6] export/fuse: Let permissions be adjustable

[Max Reitz]

Allow changing the file mode, UID, and GID through SETATTR.

Without allow_other, UID and GID are not allowed to be changed, because
it would not make sense.  Also, changing group or others' permissions
is not allowed either.

For read-only exports, +w cannot be set.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/export/fuse.c | 73 ++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 62 insertions(+), 11 deletions(-)

diff --git a/block/export/fuse.c b/block/export/fuse.c
index 26ad644cd7..ada9e263eb 100644
--- a/block/export/fuse.c
+++ b/block/export/fuse.c
@@ -48,6 +48,10 @@ typedef struct FuseExport {
     bool growable;
     /* Whether allow_other was used as a mount option or not */
     bool allow_other;
+
+    mode_t st_mode;
+    uid_t st_uid;
+    gid_t st_gid;
 } FuseExport;
 
 static GHashTable *exports;
@@ -125,6 +129,13 @@ static int fuse_export_create(BlockExport *blk_exp,
         args->allow_other = FUSE_EXPORT_ALLOW_OTHER_AUTO;
     }
 
+    exp->st_mode = S_IFREG | S_IRUSR;
+    if (exp->writable) {
+        exp->st_mode |= S_IWUSR;
+    }
+    exp->st_uid = getuid();
+    exp->st_gid = getgid();
+
     if (args->allow_other == FUSE_EXPORT_ALLOW_OTHER_AUTO) {
         /* Ignore errors on our first attempt */
         ret = setup_fuse_export(exp, args->mountpoint, true, NULL);
@@ -338,7 +349,6 @@ static void fuse_getattr(fuse_req_t req, fuse_ino_t inode,
     int64_t length, allocated_blocks;
     time_t now = time(NULL);
     FuseExport *exp = fuse_req_userdata(req);
-    mode_t mode;
 
     length = blk_getlength(exp->common.blk);
     if (length < 0) {
@@ -353,17 +363,12 @@ static void fuse_getattr(fuse_req_t req, fuse_ino_t inode,
         allocated_blocks = DIV_ROUND_UP(allocated_blocks, 512);
     }
 
-    mode = S_IFREG | S_IRUSR;
-    if (exp->writable) {
-        mode |= S_IWUSR;
-    }
-
     statbuf = (struct stat) {
         .st_ino     = inode,
-        .st_mode    = mode,
+        .st_mode    = exp->st_mode,
         .st_nlink   = 1,
-        .st_uid     = getuid(),
-        .st_gid     = getgid(),
+        .st_uid     = exp->st_uid,
+        .st_gid     = exp->st_gid,
         .st_size    = length,
         .st_blksize = blk_bs(exp->common.blk)->bl.request_alignment,
         .st_blocks  = allocated_blocks,
@@ -409,19 +414,52 @@ static int fuse_do_truncate(const FuseExport *exp, int64_t size,
 }
 
 /**
- * Let clients set file attributes.  Only resizing is supported.
+ * Let clients set file attributes.  Only resizing and changing
+ * permissions (st_mode, st_uid, st_gid) is allowed.
+ * Changing permissions is only allowed as far as it will actually
+ * permit access: Read-only exports cannot be given +w, and exports
+ * without allow_other cannot be given a different UID or GID, and
+ * they cannot be given non-owner access.
  */
 static void fuse_setattr(fuse_req_t req, fuse_ino_t inode, struct stat *statbuf,
                          int to_set, struct fuse_file_info *fi)
 {
     FuseExport *exp = fuse_req_userdata(req);
+    int supported_attrs;
     int ret;
 
-    if (to_set & ~FUSE_SET_ATTR_SIZE) {
+    supported_attrs = FUSE_SET_ATTR_SIZE | FUSE_SET_ATTR_MODE;
+    if (exp->allow_other) {
+        supported_attrs |= FUSE_SET_ATTR_UID | FUSE_SET_ATTR_GID;
+    }
+
+    if (to_set & ~supported_attrs) {
         fuse_reply_err(req, ENOTSUP);
         return;
     }
 
+    /* Do some argument checks first before committing to anything */
+    if (to_set & FUSE_SET_ATTR_MODE) {
+        /*
+         * Without allow_other, non-owners can never access the export, so do
+         * not allow setting permissions for them
+         */
+        if (!exp->allow_other &&
+            (statbuf->st_mode & (S_IRWXG | S_IRWXO)) != 0)
+        {
+            fuse_reply_err(req, EPERM);
+            return;
+        }
+
+        /* +w for read-only exports makes no sense, disallow it */
+        if (!exp->writable &&
+            (statbuf->st_mode & (S_IWUSR | S_IWGRP | S_IWOTH)) != 0)
+        {
+            fuse_reply_err(req, EROFS);
+            return;
+        }
+    }
+
     if (to_set & FUSE_SET_ATTR_SIZE) {
         if (!exp->writable) {
             fuse_reply_err(req, EACCES);
@@ -435,6 +473,19 @@ static void fuse_setattr(fuse_req_t req, fuse_ino_t inode, struct stat *statbuf,
         }
     }
 
+    if (to_set & FUSE_SET_ATTR_MODE) {
+        /* Ignore FUSE-supplied file type, only change the mode */
+        exp->st_mode = (statbuf->st_mode & 07777) | S_IFREG;
+    }
+
+    if (to_set & FUSE_SET_ATTR_UID) {
+        exp->st_uid = statbuf->st_uid;
+    }
+
+    if (to_set & FUSE_SET_ATTR_GID) {
+        exp->st_gid = statbuf->st_gid;
+    }
+
     fuse_getattr(req, inode, fi);
 }
 
-- 
2.31.1

[PATCH v2 5/6] iotests/308: Test +w on read-only FUSE exports

[Max Reitz]

Test that +w on read-only FUSE exports returns an EROFS error.  u+x on
the other hand should work.  (There is no special reason to choose u+x
here, it simply is like +w another flag that is not set by default.)

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/308     | 11 +++++++++++
 tests/qemu-iotests/308.out |  4 ++++
 2 files changed, 15 insertions(+)

diff --git a/tests/qemu-iotests/308 b/tests/qemu-iotests/308
index d13a9a969c..6b386bd523 100755
--- a/tests/qemu-iotests/308
+++ b/tests/qemu-iotests/308
@@ -170,6 +170,17 @@ fuse_export_add 'export-mp' "'mountpoint': '$EXT_MP'"
 # Check that the export presents the same data as the original image
 $QEMU_IMG compare -f raw -F $IMGFMT -U "$EXT_MP" "$TEST_IMG"
 
+# Some quick chmod tests
+stat -c 'Permissions pre-chmod: %a' "$EXT_MP"
+
+# Verify that we cannot set +w
+chmod u+w "$EXT_MP" 2>&1 | _filter_testdir | _filter_imgfmt
+stat -c 'Permissions post-+w: %a' "$EXT_MP"
+
+# But that we can set, say, +x (if we are so inclined)
+chmod u+x "$EXT_MP" 2>&1 | _filter_testdir | _filter_imgfmt
+stat -c 'Permissions post-+x: %a' "$EXT_MP"
+
 echo
 echo '=== Mount over existing file ==='
 
diff --git a/tests/qemu-iotests/308.out b/tests/qemu-iotests/308.out
index 0e9420645f..fc47bb11a2 100644
--- a/tests/qemu-iotests/308.out
+++ b/tests/qemu-iotests/308.out
@@ -50,6 +50,10 @@ wrote 67108864/67108864 bytes at offset 0
           } }
 {"return": {}}
 Images are identical.
+Permissions pre-chmod: 400
+chmod: changing permissions of 'TEST_DIR/t.IMGFMT.fuse': Read-only file system
+Permissions post-+w: 400
+Permissions post-+x: 500
 
 === Mount over existing file ===
 {'execute': 'block-export-add',
-- 
2.31.1

[PATCH v2 6/6] iotests/fuse-allow-other: Test allow-other

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/tests/fuse-allow-other     | 175 ++++++++++++++++++
 tests/qemu-iotests/tests/fuse-allow-other.out |  88 +++++++++
 2 files changed, 263 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/fuse-allow-other
 create mode 100644 tests/qemu-iotests/tests/fuse-allow-other.out

diff --git a/tests/qemu-iotests/tests/fuse-allow-other b/tests/qemu-iotests/tests/fuse-allow-other
new file mode 100755
index 0000000000..a513dbce66
--- /dev/null
+++ b/tests/qemu-iotests/tests/fuse-allow-other
@@ -0,0 +1,175 @@
+#!/usr/bin/env bash
+# group: rw
+#
+# Test FUSE exports' allow-other option
+#
+# Copyright (C) 2021 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+seq=$(basename "$0")
+echo "QA output created by $seq"
+
+status=1	# failure is the default!
+
+_cleanup()
+{
+    _cleanup_qemu
+    _cleanup_test_img
+    rm -f "$EXT_MP"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ../common.rc
+. ../common.filter
+. ../common.qemu
+
+_supported_fmt generic
+
+_supported_proto file # We create the FUSE export manually
+
+sudo -n -u nobody true || \
+    _notrun 'Password-less sudo as nobody required to test allow_other'
+
+# $1: Export ID
+# $2: Options (beyond the node-name and ID)
+# $3: Expected return value (defaults to 'return')
+# $4: Node to export (defaults to 'node-format')
+fuse_export_add()
+{
+    allow_other_not_supported='option allow_other only allowed if'
+
+    output=$(
+        success_or_failure=yes _send_qemu_cmd $QEMU_HANDLE \
+            "{'execute': 'block-export-add',
+              'arguments': {
+                  'type': 'fuse',
+                  'id': '$1',
+                  'node-name': '${4:-node-format}',
+                  $2
+              } }" \
+            "${3:-return}" \
+            "$allow_other_not_supported" \
+            | _filter_imgfmt
+    )
+
+    if echo "$output" | grep -q "$allow_other_not_supported"; then
+        # Shut down qemu gracefully so it can unmount the export
+        _send_qemu_cmd $QEMU_HANDLE \
+            "{'execute': 'quit'}" \
+            'return'
+
+        wait=yes _cleanup_qemu
+
+        _notrun "allow_other not supported"
+    fi
+
+    echo "$output"
+}
+
+EXT_MP="$TEST_DIR/fuse-export"
+
+_make_test_img 64k
+touch "$EXT_MP"
+
+echo
+echo '=== Test permissions ==='
+
+# Test that you can only change permissions on the export with allow-other=true.
+# We cannot really test the primary reason behind allow-other (i.e. to allow
+# users other than the current one access to the export), because for that we
+# would need sudo, which realistically nobody will allow this test to use.
+# What we can do is test that allow-other=true also enables default_permissions,
+# i.e. whether we can still read from the file if we remove the read permission.
+
+# $1: allow-other value ('true' or 'false')
+run_permission_test()
+{
+    _launch_qemu \
+        -blockdev \
+        "$IMGFMT,node-name=node-format,file.driver=file,file.filename=$TEST_IMG"
+
+    _send_qemu_cmd $QEMU_HANDLE \
+        "{'execute': 'qmp_capabilities'}" \
+        'return'
+
+    fuse_export_add 'export' \
+        "'mountpoint': '$EXT_MP',
+         'allow-other': '$1'"
+
+    # Should always work
+    echo '(Removing all permissions)'
+    chmod 000 "$EXT_MP" 2>&1 | _filter_testdir | _filter_imgfmt
+    stat -c 'Permissions post-chmod: %a' "$EXT_MP"
+
+    # Should always work
+    echo '(Granting u+r)'
+    chmod u+r "$EXT_MP" 2>&1 | _filter_testdir | _filter_imgfmt
+    stat -c 'Permissions post-chmod: %a' "$EXT_MP"
+
+    # Should only work with allow-other: Otherwise, no permissions can be
+    # granted to the group or others
+    echo '(Granting read permissions for everyone)'
+    chmod 444 "$EXT_MP" 2>&1 | _filter_testdir | _filter_imgfmt
+    stat -c 'Permissions post-chmod: %a' "$EXT_MP"
+
+    echo 'Doing operations as nobody:'
+    # Change to TEST_DIR, so nobody will not have to attempt a lookup
+    pushd "$TEST_DIR" >/dev/null
+
+    # This is already prevented by the permissions (without allow-other, FUSE
+    # exports always have o-r), but test it anyway
+    sudo -n -u nobody cat fuse-export >/dev/null
+
+    # If the only problem were the lack of permissions, we should still be able
+    # to stat the export as nobody; it should not work without allow-other,
+    # though
+    sudo -n -u nobody \
+        stat -c 'Permissions seen by nobody: %a' fuse-export 2>&1 \
+        | _filter_imgfmt
+
+    # To prove the point, revoke read permissions for others and try again
+    chmod o-r fuse-export 2>&1 | _filter_testdir | _filter_imgfmt
+
+    # Should fail
+    sudo -n -u nobody cat fuse-export >/dev/null
+    # Should work with allow_other
+    sudo -n -u nobody \
+        stat -c 'Permissions seen by nobody: %a' fuse-export 2>&1 \
+        | _filter_imgfmt
+
+    popd >/dev/null
+
+    _send_qemu_cmd $QEMU_HANDLE \
+        "{'execute': 'quit'}" \
+        'return'
+
+    wait=yes _cleanup_qemu
+}
+
+# 'auto' should behave exactly like 'on', because 'on' tests that
+# allow_other works (otherwise, this test is skipped)
+for ao in off on auto; do
+    echo
+    echo "--- allow-other=$ao ---"
+
+    run_permission_test "$ao"
+done
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/tests/fuse-allow-other.out b/tests/qemu-iotests/tests/fuse-allow-other.out
new file mode 100644
index 0000000000..543fa52a06
--- /dev/null
+++ b/tests/qemu-iotests/tests/fuse-allow-other.out
@@ -0,0 +1,88 @@
+QA output created by fuse-allow-other
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536
+
+=== Test permissions ===
+
+--- allow-other=off ---
+{'execute': 'qmp_capabilities'}
+{"return": {}}
+{'execute': 'block-export-add',
+              'arguments': {
+                  'type': 'fuse',
+                  'id': 'export',
+                  'node-name': 'node-format',
+                  'mountpoint': 'TEST_DIR/fuse-export',
+         'allow-other': 'off'
+              } }
+{"return": {}}
+(Removing all permissions)
+Permissions post-chmod: 0
+(Granting u+r)
+Permissions post-chmod: 400
+(Granting read permissions for everyone)
+chmod: changing permissions of 'TEST_DIR/fuse-export': Operation not permitted
+Permissions post-chmod: 400
+Doing operations as nobody:
+cat: fuse-export: Permission denied
+stat: cannot statx 'fuse-export': Permission denied
+cat: fuse-export: Permission denied
+stat: cannot statx 'fuse-export': Permission denied
+{'execute': 'quit'}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_EXPORT_DELETED", "data": {"id": "export"}}
+
+--- allow-other=on ---
+{'execute': 'qmp_capabilities'}
+{"return": {}}
+{'execute': 'block-export-add',
+              'arguments': {
+                  'type': 'fuse',
+                  'id': 'export',
+                  'node-name': 'node-format',
+                  'mountpoint': 'TEST_DIR/fuse-export',
+         'allow-other': 'on'
+              } }
+{"return": {}}
+(Removing all permissions)
+Permissions post-chmod: 0
+(Granting u+r)
+Permissions post-chmod: 400
+(Granting read permissions for everyone)
+Permissions post-chmod: 444
+Doing operations as nobody:
+Permissions seen by nobody: 444
+cat: fuse-export: Permission denied
+Permissions seen by nobody: 440
+{'execute': 'quit'}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_EXPORT_DELETED", "data": {"id": "export"}}
+
+--- allow-other=auto ---
+{'execute': 'qmp_capabilities'}
+{"return": {}}
+{'execute': 'block-export-add',
+              'arguments': {
+                  'type': 'fuse',
+                  'id': 'export',
+                  'node-name': 'node-format',
+                  'mountpoint': 'TEST_DIR/fuse-export',
+         'allow-other': 'auto'
+              } }
+{"return": {}}
+(Removing all permissions)
+Permissions post-chmod: 0
+(Granting u+r)
+Permissions post-chmod: 400
+(Granting read permissions for everyone)
+Permissions post-chmod: 444
+Doing operations as nobody:
+Permissions seen by nobody: 444
+cat: fuse-export: Permission denied
+Permissions seen by nobody: 440
+{'execute': 'quit'}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_EXPORT_DELETED", "data": {"id": "export"}}
+*** done
-- 
2.31.1

Re: [PATCH v2 2/6] export/fuse: Add allow-other option

[Kevin Wolf]

Am 25.06.2021 um 16:23 hat Max Reitz geschrieben:
> Without the allow_other mount option, no user (not even root) but the
> one who started qemu/the storage daemon can access the export.  Allow
> users to configure the export such that such accesses are possible.
> 
> While allow_other is probably what users want, we cannot make it an
> unconditional default, because passing it is only possible (for non-root
> users) if the global fuse.conf configuration file allows it.  Thus, the
> default is an 'auto' mode, in which we first try with allow_other, and
> then fall back to without.
> 
> FuseExport.allow_other reports whether allow_other was actually used as
> a mount option or not.  Currently, this information is not used, but a
> future patch will let this field decide whether e.g. an export's UID and
> GID can be changed through chmod.
> 
> One notable thing about 'auto' mode is that libfuse may print error
> messages directly to stderr, and so may fusermount (which it executes).
> Our export code cannot really filter or hide them.  Therefore, if 'auto'
> fails its first attempt and has to fall back, fusermount will print an
> error message that mounting with allow_other failed.
> 
> This behavior necessitates a change to iotest 308, namely we need to
> filter out this error message (because if the first attempt at mounting
> with allow_other succeeds, there will be no such message).
> 
> Furthermore, common.rc's _make_test_img should use allow-other=off for
> FUSE exports, because iotests generally do not need to access images
> from other users, so allow-other=on or allow-other=auto have no
> advantage.  OTOH, allow-other=on will not work on systems where
> user_allow_other is disabled, and with allow-other=auto, we get said
> error message that we would need to filter out again.  Just disabling
> allow-other is simplest.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qapi/block-export.json       | 33 ++++++++++++++++++++++++++++++++-
>  block/export/fuse.c          | 28 +++++++++++++++++++++++-----
>  tests/qemu-iotests/308       |  6 +++++-
>  tests/qemu-iotests/common.rc |  6 +++++-
>  4 files changed, 65 insertions(+), 8 deletions(-)
> 
> diff --git a/qapi/block-export.json b/qapi/block-export.json
> index e819e70cac..0ed63442a8 100644
> --- a/qapi/block-export.json
> +++ b/qapi/block-export.json
> @@ -120,6 +120,23 @@
>  	    '*logical-block-size': 'size',
>              '*num-queues': 'uint16'} }
>  
> +##
> +# @FuseExportAllowOther:
> +#
> +# Possible allow_other modes for FUSE exports.
> +#
> +# @off: Do not pass allow_other as a mount option.
> +#
> +# @on: Pass allow_other as a mount option.
> +#
> +# @auto: Try mounting with allow_other first, and if that fails, retry
> +#        without allow_other.
> +#
> +# Since: 6.1
> +##
> +{ 'enum': 'FuseExportAllowOther',
> +  'data': ['off', 'on', 'auto'] }

Why not use the generic OnOffAuto type from common.json?

But since the external interface is unaffected so we can later change
this as a code cleanup and soft freeze is approaching, I won't consider
this a blocker.

Kevin

Re: [PATCH v2 6/6] iotests/fuse-allow-other: Test allow-other

[Kevin Wolf]

Am 25.06.2021 um 16:23 hat Max Reitz geschrieben:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/tests/fuse-allow-other     | 175 ++++++++++++++++++
>  tests/qemu-iotests/tests/fuse-allow-other.out |  88 +++++++++
>  2 files changed, 263 insertions(+)
>  create mode 100755 tests/qemu-iotests/tests/fuse-allow-other
>  create mode 100644 tests/qemu-iotests/tests/fuse-allow-other.out
> 
> diff --git a/tests/qemu-iotests/tests/fuse-allow-other b/tests/qemu-iotests/tests/fuse-allow-other
> new file mode 100755
> index 0000000000..a513dbce66
> --- /dev/null
> +++ b/tests/qemu-iotests/tests/fuse-allow-other
> @@ -0,0 +1,175 @@
> +#!/usr/bin/env bash
> +# group: rw
> +#
> +# Test FUSE exports' allow-other option
> +#
> +# Copyright (C) 2021 Red Hat, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +seq=$(basename "$0")
> +echo "QA output created by $seq"
> +
> +status=1	# failure is the default!
> +
> +_cleanup()
> +{
> +    _cleanup_qemu
> +    _cleanup_test_img
> +    rm -f "$EXT_MP"
> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +# get standard environment, filters and checks
> +. ../common.rc
> +. ../common.filter
> +. ../common.qemu
> +
> +_supported_fmt generic
> +
> +_supported_proto file # We create the FUSE export manually
> +
> +sudo -n -u nobody true || \
> +    _notrun 'Password-less sudo as nobody required to test allow_other'
> +
> +# $1: Export ID
> +# $2: Options (beyond the node-name and ID)
> +# $3: Expected return value (defaults to 'return')
> +# $4: Node to export (defaults to 'node-format')
> +fuse_export_add()
> +{
> +    allow_other_not_supported='option allow_other only allowed if'
> +
> +    output=$(
> +        success_or_failure=yes _send_qemu_cmd $QEMU_HANDLE \
> +            "{'execute': 'block-export-add',
> +              'arguments': {
> +                  'type': 'fuse',
> +                  'id': '$1',
> +                  'node-name': '${4:-node-format}',
> +                  $2
> +              } }" \
> +            "${3:-return}" \
> +            "$allow_other_not_supported" \
> +            | _filter_imgfmt
> +    )
> +
> +    if echo "$output" | grep -q "$allow_other_not_supported"; then
> +        # Shut down qemu gracefully so it can unmount the export
> +        _send_qemu_cmd $QEMU_HANDLE \
> +            "{'execute': 'quit'}" \
> +            'return'
> +
> +        wait=yes _cleanup_qemu
> +
> +        _notrun "allow_other not supported"
> +    fi
> +
> +    echo "$output"
> +}
> +
> +EXT_MP="$TEST_DIR/fuse-export"
> +
> +_make_test_img 64k
> +touch "$EXT_MP"
> +
> +echo
> +echo '=== Test permissions ==='
> +
> +# Test that you can only change permissions on the export with allow-other=true.
> +# We cannot really test the primary reason behind allow-other (i.e. to allow
> +# users other than the current one access to the export), because for that we
> +# would need sudo, which realistically nobody will allow this test to use.
> +# What we can do is test that allow-other=true also enables default_permissions,
> +# i.e. whether we can still read from the file if we remove the read permission.

I don't think this comment is accurate any more now that you're actually
using sudo.

> +# $1: allow-other value ('true' or 'false')

on/off/auto, actually.

I can fix this up while applying, removing the comment block above, and
adjusting this line.

Kevin

Re: [PATCH v2 0/6] export/fuse: Allow other users access to the export

[Kevin Wolf]

Am 25.06.2021 um 16:23 hat Max Reitz geschrieben:
> Max Reitz (6):
>   export/fuse: Pass default_permissions for mount
>   export/fuse: Add allow-other option
>   export/fuse: Give SET_ATTR_SIZE its own branch
>   export/fuse: Let permissions be adjustable
>   iotests/308: Test +w on read-only FUSE exports
>   iotests/fuse-allow-other: Test allow-other

Thanks, fixed up the comments in the test case as indicated and applied
to the block branch.

Kevin