ceph2: Testing the new ceph module shows a failure in smb2.session.reauth

[spuiuk]

I have been running the smbtorture test suite in sit-test-cases against the ceph2 module and notice a failure in smb2.session.reauth

This is not strictly a sit-test-case issue but a good place to log the problem.

The test setup is as follows

Share cephfs-vfs: Exports a ceph filesystem using the module ceph. Share cephfs-vfs-ll: Exports the same ceph filesystem using the module ceph2

Summary of the tests

FAILED testcases/smbtorture/test_smbtorture.py::test_smbtorture[cephfs-vfs-ll-smb2.session.reauth4] - Failed: Failure in running test - smb2.session.reauth4
PASSED testcases/smbtorture/test_smbtorture.py::test_smbtorture[cephfs-vfs-smb2.session.reauth4]

More detailed information on failure:

Failure in running test - smb2.session.reauth4
-------------------------------------------------------------------- Captured stdout call --------------------------------------------------------------------
Command: /bin/smbtorture --fullname --option=torture:progress=no --option=torture:sharedelay=100000 --option=torture:writetimeupdatedelay=500000 --format=subunit --target=samba3 --user=test1%x //samba-cephfs/cephfs-vfs-ll smb2.session.reauth4|/usr/bin/python3 /workspaces/samba_cephfs/sit-test-cases/testcases/smbtorture/selftest/filter-subunit --fail-on-empty --prefix=samba3. --expected-failures=/workspaces/samba_cephfs/sit-test-cases/testcases/smbtorture/selftest/knownfail --expected-failures=/workspaces/samba_cephfs/sit-test-cases/testcases/smbtorture/selftest/knownfail.d --expected-failures=/workspaces/samba_cephfs/sit-test-cases/testcases/smbtorture/selftest/expectedfail.d --flapping=/workspaces/samba_cephfs/sit-test-cases/testcases/smbtorture/selftest/flapping --flapping=/workspaces/samba_cephfs/sit-test-cases/testcases/smbtorture/selftest/flapping.d --flapping=/workspaces/samba_cephfs/sit-test-cases/testcases/smbtorture/selftest/flapping.cephfs|/usr/bin/python3 /workspaces/samba_cephfs/sit-test-cases/testcases/smbtorture/selftest/format-subunit

smbtorture 4.21.0pre1-GIT-914a7eb96cd
Using seed 1718107513
time: 2024-06-11 12:05:13.271322Z
test: samba3.smb2.session.reauth4
time: 2024-06-11 12:05:13.271652Z
time: 2024-06-11 12:05:13.305914Z
failure: samba3.smb2.session.reauth4 [
Exception: ../../source4/torture/smb2/session.c:603: status was NT_STATUS_ACCESS_DENIED, expected NT_STATUS_OK: smb2_setinfo_file failed

]

smbtorture 4.21.0pre1-GIT-914a7eb96cd
Using seed 1718107513
F
FAILED (1 failures, 0 errors and 0 unexpected successes in 0 testsuites)

There might be more detail in ./subunit or ./summary.

[spuiuk]

The failure occurs at line https://gitlab.com/samba-team/samba/-/blob/master/source4/torture/smb2/session.c?ref_type=heads#L602

test_session_reauth4()

• SESSION setup with user
• CREATE file
• GETINFO SEC_INFO on the file - get owner, group and dacl
• reauthticate as anonymous - SESSION setup with anonymous user
• SETINFO on file - We copy the DACL from the earlier GETINFO and add additional access for anonymous user.
• reauthenticate as original user - SESSION setup with test user.
• GETINFO on the same file

The failure in our case in in the SETINFO as an anonymous user. This step works when done using the original vfs_ceph module.

[spuiuk]

The error appears to happen at

static int vfs_ceph2_setxattr(const struct vfs_handle_struct *handle,
                  const struct vfs_ceph_iref *iref,
                  const char *name,
                  const void *value,
                  size_t size,
                  int flags)
{
..
    ret = ceph_ll_setxattr(cmount_of(handle),
                   iref->inode,
                   name,
                   value,
                   size,
                   flags,
                   perms);
..
}

The error returned is -13 -EACCES

[spuiuk]

By adding debug statements, at the point of failure, I can confirm that the perms set in vfs_ceph_fsetxattr()->vfs_ceph2_setxattr()->vfs_ceph_userperm_new() set uid/gid set to 65534/65534. This coincides with the session setup with anonymous user.

[synarete]

By adding debug statements, at the point of failure, I can confirm that the perms set in vfs_ceph_fsetxattr()->vfs_ceph2_setxattr()->vfs_ceph_userperm_new() set uid/gid set to 65534/65534. This coincides with the session setup with anonymous user.

@spuiuk In the above sequence, upon CREATE file, what are the permissions of this file? Does it allow uid/gid 65534/65534 to modify the file? If not, then the behavior of the above vfs_ceph2_setxattr is correct (and the bug is in vfs_ceph which allows setting xattr by the wrong user).

[spuiuk]

It appears that the smbtorture test expects to be able to modify the DACL as anonymous user. the vfs_ceph module may have a bug, but it appears to result in the smbtorture test pass.

My next steps: 1) Identify the default operations performed by the vfs layer is 2) Check with upstream how this is expected to work.

[synarete]

Maybe also some specific settings in smb.conf?

[spuiuk]

I added the following debug calls to the vfs_ceph2 module

--- a/source3/modules/vfs_ceph2.c
+++ b/source3/modules/vfs_ceph2.c
@@ -530,6 +530,8 @@ static struct UserPerm *vfs_ceph_userperm_new(
        const struct security_unix_token *unix_token = NULL;

        unix_token = handle->conn->session_info->unix_token;
+       DEBUG(0, ("SP: %s:%d token: uid %d gid %d process: uid %d gid %d\n", __func__, __LINE__, unix_token->uid, unix_token->gid, geteuid(), getegid()));
+
        return ceph_userperm_new(unix_token->uid,
                                 unix_token->gid,
                                 unix_token->ngroups,
@@ -1043,6 +1045,7 @@ static int vfs_ceph2_setxattr(const struct vfs_handle_struct *handle,
                               size,
                               flags,
                               perms);
+       DEBUG(0, ("SP: %s:%d ret %d\n", __func__, __LINE__, ret));
        vfs_ceph_userperm_del(perms);
        return ret;
 }

Which gave the following output

SP: vfs_ceph_userperm_new:533 token: uid 65534 gid 65534 process: uid 0 gid 0
SP: vfs_ceph2_setxattr:1048 ret -13

ie. even though the uid/gid is set to nobody/nobody, the effective uid/gid is 0.

The call to vfs_ceph_userperm_new() uses nobody/nobody to set the perms. Maybe we should use euid and egid instead to set the permissions?

[synarete]

All looks as expected: the in-coming request has proper user credentials (uid/gid=65534/65534) while the smbd process serves the requests (for some reason) as root.

The interesting question is how local linux filesystem (e.g., xfs) behaves under this smbtorture test.

[spuiuk]

static int vfswrap_fsetxattr(struct vfs_handle_struct *handle, struct files_struct *fsp, const char *name, const void *value, size_t size, int flags)
{
    int fd = fsp_get_pathref_fd(fsp);

    SMB_ASSERT(!fsp_is_alternate_stream(fsp));

    if (!fsp->fsp_flags.is_pathref) {
        return fsetxattr(fd, name, value, size, flags);
    }

    if (fsp->fsp_flags.have_proc_fds) {
        struct sys_proc_fd_path_buf buf;

        return setxattr(sys_proc_fd_path(fd, &buf),
                name,
                value,
                size,
                flags);
    }

    /*
     * This is no longer a handle based call.
     */
    return setxattr(fsp->fsp_name->base_name, name, value, size, flags);
}

The kernel will use the euid/egid when calling fsetxattr/setxattr() to set the attributes.

[spuiuk]

commit d490629b6b29a7580b8a6a737719b9d82a0ebb06 (HEAD)
Author: Sachin Prabhu <sprabhu@redhat.com>
Date:   Thu Jun 13 14:31:12 2024 +0100

    vfs_ceph2: Use euid when setting perms

    We use the values set in handle->conn->session_info->unix_token
    to set the perms using ceph_userperm_new(). This could cause issues if a
    different euid is set. In this case, use the euid and egid values when making a
    call to ceph_userperm_new().

    The bug was caught when testing the smbtorture test
    smb2.session.reauth4
    where a session setup to authenticate as anonymous user is performed before the
    DACL is setup. This results in uid/gid set to nobody/nobody causing the
    operation to fail.

    Signed-off-by: Sachin Prabhu <sp@spui.uk>

diff --git a/source3/modules/vfs_ceph2.c b/source3/modules/vfs_ceph2.c
index 98f3c8e2217..b1149407529 100644
--- a/source3/modules/vfs_ceph2.c
+++ b/source3/modules/vfs_ceph2.c
@@ -528,12 +528,26 @@ static struct UserPerm *vfs_ceph_userperm_new(
        const struct vfs_handle_struct *handle)
 {
        const struct security_unix_token *unix_token = NULL;
+       uid_t uid = geteuid();
+       gid_t gid;
+       uint32_t ngroups;
+       gid_t *groups;

        unix_token = handle->conn->session_info->unix_token;
-       return ceph_userperm_new(unix_token->uid,
-                                unix_token->gid,
-                                unix_token->ngroups,
-                                unix_token->groups);
+       if (unix_token->uid == uid) {
+               gid = unix_token->gid;
+               ngroups = unix_token->ngroups;
+               groups = unix_token->groups;
+       } else {
+               gid = getegid();
+               ngroups = 0;
+               groups = NULL;
+       }
+
+       return ceph_userperm_new(uid,
+                               gid,
+                               ngroups,
+                               groups);
 }

 static void vfs_ceph_userperm_del(struct UserPerm *perms)

I just tested using the following patch and was able to successfully the smbtorture tests setup in sit-test-cases.

[xhernandez]

I'm not sure if using the euid/egid is the right solution. If the process is running as root, the unix token should have also been updated. If the unix_token is not consistent with the uid/gid of the process, I think something is wrong.

Considering that distributed filesystems like Ceph don't really need to change the process owner to work correctly, retrieving the "good" user from a system call instead of the unix_token seems problematic to me.

The question should be why the unix_token has not been updated when the process owner has been changed ?

[anoopcs9]

David anticipated this exact issue while reviewing the oldest attempt from @synarete to add support for supplementary groups in _vfsceph. See the comment !3466(comment 1718058243).

In the light of recent discussions let me try to consolidate the findings:

• Test situation involves _vfs_aclxattr module loaded.
• become_root() was needed under the assumption(on local file systems) that only root can perform setxattr("security.NTACL",...).
  • with CephFS it is not the case.
• Under CephFS if not root the initial user who created the file can also perform setxattr("security.NTACL",...) but the particular test re-authenticate as anonymous user(SID_NT_ANONYMOUS).
• _vfsceph end up falling back to geteuid()/getegid() mechanism ultimately performing the operation(setxattr("security.NTACL",...)) as root and thus smb2.session.reauth4 is successfully completed.
• Our expectation on struct security_unix_token to be populated with the information of user performing the task/operation at a given point in time doesn't hold true when become_root() is called in this context -- why?
  • anonymous re-authentication changes struct security_unix_token to nobody/nobody(mapped from SID_NT_ANONYMOUS) but stays intact even after become_root().

[synarete]

David anticipated this exact issue while reviewing the oldest attempt from @synarete to add support for supplementary groups in _vfsceph. See the comment !3466(comment 1718058243).

In the light of recent discussions let me try to consolidate the findings:

• Test situation involves _vfs_aclxattr module loaded.

• become_root() was needed under the assumption(on local file systems) that only root can perform setxattr("security.NTACL",...).

  • with CephFS it is not the case.
• Under CephFS if not root the initial user who created the file can also perform setxattr("security.NTACL",...) but the particular test re-authenticate as anonymous user(SID_NT_ANONYMOUS).
• _vfsceph end up falling back to geteuid()/getegid() mechanism ultimately performing the operation(setxattr("security.NTACL",...)) as root and thus smb2.session.reauth4 is successfully completed.

• Our expectation on struct security_unix_token to be populated with the information of user performing the task/operation at a given point in time doesn't hold true when become_root() is called in this context -- why?

  • anonymous re-authentication changes struct security_unix_token to nobody/nobody(mapped from SID_NT_ANONYMOUS) but stays intact even after become_root().

@anoopcs9 Excellent analysis, thanks! However, what is the proper solution? How do we tell Samba's VFS that cephfs does not need this become_root() ?

[anoopcs9]

David anticipated this exact issue while reviewing the oldest attempt from @synarete to add support for supplementary groups in _vfsceph. See the comment !3466(comment 1718058243). In the light of recent discussions let me try to consolidate the findings:

• Test situation involves _vfs_aclxattr module loaded.

• become_root() was needed under the assumption(on local file systems) that only root can perform setxattr("security.NTACL",...).

  • with CephFS it is not the case.
• Under CephFS if not root the initial user who created the file can also perform setxattr("security.NTACL",...) but the particular test re-authenticate as anonymous user(SID_NT_ANONYMOUS).
• _vfsceph end up falling back to geteuid()/getegid() mechanism ultimately performing the operation(setxattr("security.NTACL",...)) as root and thus smb2.session.reauth4 is successfully completed.

• Our expectation on struct security_unix_token to be populated with the information of user performing the task/operation at a given point in time doesn't hold true when become_root() is called in this context -- why?

  • anonymous re-authentication changes struct security_unix_token to nobody/nobody(mapped from SID_NT_ANONYMOUS) but stays intact even after become_root().

@anoopcs9 Excellent analysis, thanks! However, what is the proper solution? How do we tell Samba's VFS that cephfs does not need this become_root() ?

I think metze(Stefan Metzmacher) replied to the thread which Sachin initiated on samba-technical with a way forward where he suggested to make use of VFS FSP extension to store the owner info(and file handle) as re-authentication doesn't seem to have an effect on open file handles. This is the exactly the context with smb2.session.reauth4 where re-authentication as anonymous user happens while the file handle is still open. While we decide whether to pursue the suggested solution you may look at how FSP extensions are used in _vfsglusterfs(for storing real Gluster fd; glfds) as an example.

[xhernandez]

I think there are two things to consider here:

1. Currently, the _vfs_cephll branch uses ceph_ll_setxattr().

   This means that even if a file handle is open, explicit permissions need to be passed, which is problematic. Probably the solution here would be to use ceph_fsetxattr() when a non-path-ref file handle is passed. One important thing to consider in this case is if are we sure that the vfs module will receive a non-path-ref file handle if and only if it comes form a user's explicitly open file handle. Otherwise there could be other permission issues.

2. For kernel mounted shares, setting NTACL requires root permissions. Non-kernel mounted shares could have similar requirements.

   A regular kernel mounted filesystem doesn't allow to modify the security.NTACL xattr even if the user has write permissions on the file (that's why acl_xattr vfs module uses become_root()). While I think this is not a problem for Ceph right now (not fully sure, though. However, if it's true, is it a bug ?), the fact that become_root() only changes the effective uid/gid of the process could cause the test to continue failing even if it's using the original permissions of the user that opened the file.

[spuiuk]

So summarising Metze's reply.

The test smb2.session.reauth4 is used to check that re-authentication doesn't affect the permissions on an open filehandle. ie. the unix_token set at the time of CREATE should be used for operations. So even after we reauthenticate as Anonymous, we should use the uid/gid used when we opened the file.

We can possibly do this by storing the perms returned from vfs_ceph_userperm_new along with the vfs_ceph_mnt_entry(?) data pointed to by handle->data.

[xhernandez]

So summarising Metze's reply.

The test smb2.session.reauth4 is used to check that re-authentication doesn't affect the permissions on an open filehandle. ie. the unix_token set at the time of CREATE should be used for operations. So even after we reauthenticate as Anonymous, we should use the uid/gid used when we opened the file.

We can possibly do this by storing the perms returned from vfs_ceph_userperm_new along with the vfs_ceph_mnt_entry(?) data pointed to by handle->data.

That's not necessary. We just need to call ceph_fsetxattr() with the Ceph file handle saved in the fsp at the time of open. However even doing this the test could continue failing if Ceph requires root privileges to modify the "security" namespace of the xattrs.

For Ceph it might work if it doesn't have this requirement but, conceptually, having inconsistent owners between unix_token and process owner is an issue that could cause other problems, IMO.

Having to use system calls to recover the uid/gid we have just set, seems totally unnecessary to me.

[synarete]

So summarising Metze's reply.

The test smb2.session.reauth4 is used to check that re-authentication doesn't affect the permissions on an open filehandle. ie. the unix_token set at the time of CREATE should be used for operations. So even after we reauthenticate as Anonymous, we should use the uid/gid used when we opened the file.

We can possibly do this by storing the perms returned from vfs_ceph_userperm_new along with the vfs_ceph_mnt_entry(?) data pointed to by handle->data.

Indeed: the proper solution if to embed libcephfs' UserPerm* as part of of the fsp-extension upon vfs_ceph_openat, and use it in all fsp based calls. I am already working on this one (it is not so trivial and need to think carefully how to reorder the code). That said, @xhernandez comment from above is related to the case of calling VFS fsetxattr_fn hook without an fsp. For this problem I still don't have a valid solution.

In all odds: @spuiuk and @anoopcs9 : many thanks for finding and digging into this one. You spotted a fundamental design flaw.

[xhernandez]

So summarising Metze's reply. The test smb2.session.reauth4 is used to check that re-authentication doesn't affect the permissions on an open filehandle. ie. the unix_token set at the time of CREATE should be used for operations. So even after we reauthenticate as Anonymous, we should use the uid/gid used when we opened the file. We can possibly do this by storing the perms returned from vfs_ceph_userperm_new along with the vfs_ceph_mnt_entry(?) data pointed to by handle->data.

Indeed: the proper solution if to embed libcephfs' UserPerm* as part of of the fsp-extension upon vfs_ceph_openat, and use it in all fsp based calls. I am already working on this one (it is not so trivial and need to think carefully how to reorder the code). That said, @xhernandez comment from above is related to the case of calling VFS fsetxattr_fn hook without an fsp. For this problem I still don't have a valid solution.

If you are right, I think I'm missing something. If we already have the associated Ceph file handle in the fsp from the time when the file was opened, then why do we need to also store the user permissions and complicate the logic ? Instead of using the inode-based ceph_ll_setxattr(), which uses explicit permissions, we just need to use the file handle-based ceph_fsetxattr() which implicitly uses the permissions for the file handle.

[synarete]

So summarising Metze's reply. The test smb2.session.reauth4 is used to check that re-authentication doesn't affect the permissions on an open filehandle. ie. the unix_token set at the time of CREATE should be used for operations. So even after we reauthenticate as Anonymous, we should use the uid/gid used when we opened the file. We can possibly do this by storing the perms returned from vfs_ceph_userperm_new along with the vfs_ceph_mnt_entry(?) data pointed to by handle->data.

Indeed: the proper solution if to embed libcephfs' UserPerm* as part of of the fsp-extension upon vfs_ceph_openat, and use it in all fsp based calls. I am already working on this one (it is not so trivial and need to think carefully how to reorder the code). That said, @xhernandez comment from above is related to the case of calling VFS fsetxattr_fn hook without an fsp. For this problem I still don't have a valid solution.

If you are right, I think I'm missing something. If we already have the associated Ceph file handle in the fsp from the time when the file was opened, then why do we need to also store the user permissions and complicate the logic ? Instead of using the inode-based ceph_ll_setxattr(), which uses explicit permissions, we just need to use the file handler-based ceph_fsetxattr() which implicitly uses the permissions for the file handle.

The problem is with the case where we don't have fsp upon setxattr.

[xhernandez]

The problem is with the case where we don't have fsp upon setxattr.

If we have fsp, why the fix is to store user permissions at the time of open ? we already have the fsp which has the Ceph file handle associated, right ? so just use it with ceph_fsetxattr().

If we don't have fsp, then fsetxattr is illegal. We should only use setxattr in this case, and this requires a valid unix_token that is consistent with the process owner.

[synarete]

The problem is with the case where we don't have fsp upon setxattr.

If we have fsp, why the fix is to store user permissions at the time of open ? we already have the fsp which has the Ceph file handle associated, right ? so just use it with ceph_fsetxattr().

If we don't have fsp, then fsetxattr is illegal. We should only use setxattr in this case, and this requires a valid unix_token that is consistent with the process owner.

Lets look at simple example: vfs_ceph_ftruncate has an open fsp, but it calls vfs_ceph2_truncate with its associate file-ref. Now, vfs_ceph2_truncate creates UserPerm* on the fly, based on current caller unix_token instead of the one used upon openat_fn. This need to be fixed (work-in-progress).

[anoopcs9]

So summarising Metze's reply. The test smb2.session.reauth4 is used to check that re-authentication doesn't affect the permissions on an open filehandle. ie. the unix_token set at the time of CREATE should be used for operations. So even after we reauthenticate as Anonymous, we should use the uid/gid used when we opened the file. We can possibly do this by storing the perms returned from vfs_ceph_userperm_new along with the vfs_ceph_mnt_entry(?) data pointed to by handle->data.

That's not necessary. We just need to call ceph_fsetxattr() with the Ceph file handle saved in the fsp at the time of open. However even doing this the test could continue failing if Ceph requires root privileges to modify the "security" namespace of the xattrs.

If I remember correctly I did some experiments in the past with a patch set that doesn't use _becomeroot()(but just CAP_DAC_OVERRIDE) in Samba(with _vfs_aclxattr) and _setxattr("security.NTACL",...)_ was successful on CephFS via _vfsceph.

having inconsistent owners between unix_token and process owner is an issue that could cause other problems, IMO.

I agree.

[xhernandez]

The problem is with the case where we don't have fsp upon setxattr.

If we have fsp, why the fix is to store user permissions at the time of open ? we already have the fsp which has the Ceph file handle associated, right ? so just use it with ceph_fsetxattr(). If we don't have fsp, then fsetxattr is illegal. We should only use setxattr in this case, and this requires a valid unix_token that is consistent with the process owner.

Lets look at simple example: vfs_ceph_ftruncate has an open fsp, but it calls vfs_ceph2_truncate with its associate file-ref. Now, vfs_ceph2_truncate creates UserPerm* on the fly, based on current caller unix_token instead of the one used upon openat_fn. This need to be fixed (work-in-progress).

Oh, I think now I see what I was missing. libcephfs lacks file-handle-based low-level operations. The high-level ones only accept an integer fd instead of a real file-handle, which is what's needed. I think this should be fixed in the libcephfs side. libcephfs already exports real file handles, which are used in ceph_ll_read(), for example. I think there should exist other low-level functions that also receive a file-handle, like ceph_ll_fsetxattr() and ceph_ll_ftruncate() (and possibly others).

In the particular case of truncate, even there isn't a low-level function for it.

Shouldn't we try to add all these functions in libcephfs ?

[xhernandez]

So summarising Metze's reply. The test smb2.session.reauth4 is used to check that re-authentication doesn't affect the permissions on an open filehandle. ie. the unix_token set at the time of CREATE should be used for operations. So even after we reauthenticate as Anonymous, we should use the uid/gid used when we opened the file. We can possibly do this by storing the perms returned from vfs_ceph_userperm_new along with the vfs_ceph_mnt_entry(?) data pointed to by handle->data.

That's not necessary. We just need to call ceph_fsetxattr() with the Ceph file handle saved in the fsp at the time of open. However even doing this the test could continue failing if Ceph requires root privileges to modify the "security" namespace of the xattrs.

If I remember correctly I did some experiments in the past with a patch set that doesn't use _becomeroot()(but just CAP_DAC_OVERRIDE) in Samba(with _vfs_aclxattr) and _setxattr("security.NTACL",...)_ was successful on CephFS via _vfsceph.

having inconsistent owners between unix_token and process owner is an issue that could cause other problems, IMO.

I agree.

Metze has just replied to my email to samba-technical. There's a get_current_utok() that returns the effective user token that matches the effective onwer of the process.

Probably we should consider using it in (almost ?) all places instead of conn->session_info->unix_token.

[xhernandez]

Metze has just replied to my email to samba-technical. There's a get_current_utok() that returns the effective user token that matches the effective onwer of the process.

Probably we should consider using it in (almost ?) all places instead of conn->session_info->unix_token.

BTW, this doesn't fix the issue about the permissions at the time of opening the file. We still need to fix that, but at least we have a way to get the effective owner for the cases where it's really needed.

[synarete]

et_current_utok

Thanks, will do so.

[spuiuk]

Ran a quick test with more instrumentation

+         DEBUG(0, ("SP: %s:%d getuid uid/gid %d/%d\n", __func__, __LINE__, getuid(), getgid()));
+         DEBUG(0, ("SP: %s:%d geteuid uid/gid %d/%d\n", __func__, __LINE__, getuid(), getegid()));
+         ut = handle->conn->session_info->unix_token;        
+         DEBUG(0, ("SP: %s:%d unix_token uid/gid %d/%d\n", __func__, __LINE__, ut->uid, ut->gid)); 
+         ut = get_current_utok(handle->conn);                      
+         DEBUG(0, ("SP: %s:%d current_ut uid/gid %d/%d\n", __func__, __LINE__, ut->uid, ut->gid));

SP: vfs_ceph2_setxattr:1036 getuid uid/gid 0/0
SP: vfs_ceph2_setxattr:1037 geteuid uid/gid 0/0
SP: vfs_ceph2_setxattr:1039 unix_token uid/gid 65534/65534
SP: vfs_ceph2_setxattr:1041 current_ut uid/gid 0/0

The output above is when the user is logged in as anonymous.

[anoopcs9]

Metze has just replied to my email to samba-technical. There's a get_current_utok() that returns the effective user token that matches the effective onwer of the process. Probably we should consider using it in (almost ?) all places instead of conn->session_info->unix_token.

BTW, this doesn't fix the issue about the permissions at the time of opening the file.

What is the issue w.r.t permissions during file open?

[xhernandez]

BTW, this doesn't fix the issue about the permissions at the time of opening the file.

What is the issue w.r.t permissions during file open?

My sentence wasn't very clear, sorry. I meant the issue about caching the user permissions at the time of opening a file to use them later.

The problem is that even using get_current_utok() the permissions can be incorrect in some cases.

In general, the open is done with some user. Then if we switch to the anonymous user and attempt to set a normal extended attribute (not security.NTACL), it's expected that the operation will succeed or fail depending on the rights of the original user, not the rights of the anonymous user. So the right thing to do here is to use the permissions of the user that opened the file (or just use the file handle, ideally).

However, when the modified xattr is security.NTACL, even the "normal" approach is not correct. For this particular case we should use get_current_utok(). So, ideally, if become_root() has been called, we should use the get_current_utok() and use the corresponding inode-based low-level libcephfs function. Otherwise we should use the file-handle-based low-level libcephfs function or use the cached permissions at open time if that low-level function doesn't exist (as it happens now).

[anoopcs9]

Let me try to consolidate the approach:

At least with CephFS(using libcephfs), until all required file handle based low-level APIs are available, we could use cached permissions always to perform various operations on an open file handle(fsp) provided CephFS doesn't stop us in situations like dealing with xattrs from security namespace. Or to be on safer side we may insert an additional check using _get_currentutok() and accordingly consume those credentials.

Please feel free to correct or add more to it.

[xhernandez]

Let me try to consolidate the approach:

At least with CephFS(using libcephfs), until all required file handle based low-level APIs are available, we could use cached permissions always to perform various operations on an open file handle(fsp) provided CephFS doesn't stop us in situations like dealing with xattrs from security namespace.

Correct. That's what I think.

Or to be on safer side we may insert an additional check using _get_currentutok() and accordingly consume those credentials.

What kind of check are you thinking about ?

The credentials returned by get_current_utok() will match the current owner of the process, but they may not match the user that opened the file. get_current_utok() is only required if/when ceph doesn't allow modifications of xattrs in the security namespace to normal users. In general, for file-handle-based operations, we should use the cached permissions at the time of open until the corresponding file-handle-based low-level function is provided by libcephfs.

[synarete]

Let me try to consolidate the approach: At least with CephFS(using libcephfs), until all required file handle based low-level APIs are available, we could use cached permissions always to perform various operations on an open file handle(fsp) provided CephFS doesn't stop us in situations like dealing with xattrs from security namespace.

Correct. That's what I think.

Or to be on safer side we may insert an additional check using _get_currentutok() and accordingly consume those credentials.

What kind of check are you thinking about ?

The credentials returned by get_current_utok() will match the current owner of the process, but they may not match the user that opened the file. get_current_utok() is only required if/when ceph doesn't allow modifications of xattrs in the security namespace to normal users. In general, for file-handle-based operations, we should use the cached permissions at the time of open until the corresponding file-handle-based low-level function is provided by libcephfs.

That is correct, and will be implemented as part of next code iteration (vfs_ceph_new) . Specifically, libcephfs' UserPerm is now part of vfs_ceph_fh and is set when adding fsp-extension. Please note that this is still a work-in-progress.

[anoopcs9]

Or to be on safer side we may insert an additional check using _get_currentutok() and accordingly consume those credentials.

What kind of check are you thinking about ?

The credentials returned by get_current_utok() will match the current owner of the process, but they may not match the user that opened the file. get_current_utok() is only required if/when ceph doesn't allow modifications of xattrs in the security namespace to normal users.

Exactly, that's the hypothetical situation I had in mind where super user privileges might be required to perform some operation. Remember, we are yet to decide whether the current behaviour of CephFS returning success for setxattr("security.NTACL",...) by a non-root user(even if its the owner) is correct or not. If not setxattr("security.NTACL",...) some operation(if any) requiring super user privileges will have to check using get_current_utok() to allow privilege escalation.