mds/Locker: remove session check access when doing cap updates#47506
mds/Locker: remove session check access when doing cap updates#47506
Conversation
|
With this fix, a fuse client with root_squash MDS caps worked as expected (as before). When using a kclient with root_squash MDS caps, the data written by a non-root user wasn't lost after remounting the client or clearing the client's cache. A root user wasn't able to create, remove, truncate a file, but was able to write to the file ( During write as a root user from the kclient, I see the following in MDS log, where I suspect the write is allowed to succeed. Writing as root user from a fuse client with root_squash MDS caps fails as expected. I'm trying to compare fuse client and kclient calls to the MDS when trying to write to a file. |
gregsfortytwo
left a comment
There was a problem hiding this comment.
During write as a root user from the kclient, I see the following in MDS log, where I suspect the write is allowed to succeed.
This seems not okay.
This is not always true. There have cases will set them, for example when truncating a file with a larger size, it will set the
The logic of this between user space client and kclient is different and the kclient will always use the |
IMO it should fail when opening the file with a |
Please take care of the async creating. You can try to disable the |
@lxbsz Are you saying it's not always the case that the user space client sends cap updates as m->caller_uid:m->caller_gid as -1:-1 as mentioned in the commit message? I will fix the commit message. If we consider the example that you mention, truncating a file using a FUSE client, would the change I made in the MDSLocker code affect the existing behavior when truncating a file?
Sorry, I'm unable to understand the above statement. Are you saying that kclient uses inode->i_uid/i_gid to track caller_uid and caller_gid when requesting cap updates? Based on the above kclient code, fs/ceph/caps.c, I meant to say that caller_uid and caller_gid are always set as 0:0 when updating caps. |
Got it. With this PR, when using a FUSE client with root_squash MDS auth caps, I cannot write to file as a root user ( |
Yeah, correct.
I think it's okay. In that case the client will buffer that changes and just assume the operation is successful. IMO we shouldn't defer checking the permission in the MDS when flushing the caps and it should be fail before buffering the changes instead.
You are right. I think I was checking another code path for kclient. |
Still not sure about this yet. If you have logs that will be very easy to find the root cause. |
@lxbsz , I've added the logs below. Please let me know if you need more details. Built Ceph (main branch) including the change in this PR. Mounted a CephFS volume using a FUSE client with root squash MDS caps. I was unable to write to an existing file as a root user ( The relevant MDS log during "sudo vim test-fuse-client.txt" can be downloaded from, Mounted the same CephFS volume using a kernel client with root squash MDS caps. I was able to write to an existing file as a root user ( The relevant MDS log during "sudo vim test-kclient.txt" can be downloaded from, I grepped the MDS logs (shared above) for 'reply_client_request' to understand how FUSE client requests differs from kclient requests during The MDS replies to FUSE client requests, The MDS reply to kclient requests during From the above you can see that the last setattr requested by kclient is denied by the MDS. Later in the MDS log, I see the following |
|
This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved |
|
@ajarr This looks good to me. Please rebase the PR and remove the draft. |
Wait. Checked it again as we discussed above, for the setattr case if the client will buffer the changes it won't check the permission and will defer checking it when flushing the cap as it's doing here. So we still need make sure this won't introduce issues such as when a non-permitted user could update it ? As mentioned above we may need to check the permission before buffering it ? |
|
For fuse client:
The While for the kernel client there was no
It seems the open succeeded in the client side directly. I need to check the kernel code later to check why. Though the backup file also failed to create but the |
That sounds like the correct approach IMO. |
If so we need to do the same check in client side anyway ? |
When open a existing file and if the inode is cached in client side: For the 1, Try to open the inode and check the caps issued to see whether the For the 1, Try to open the inode and check the caps issued to see whether the This is why the We need to find a way to force check this in kernel. |
The kernel patch fixed it. |
That's one issue. The other (probably a bit more important) thing is that I expected the flush/sync to error out, but that doesn't happen - do we know why?
|
|
...
With this PR after being remounted the changed data will be lost. While actually the contents have already written to the data pool, just the metadatas are lost in MDS when flushing the caps. If with this PR, I can see it was wrote normally. |
I've removed the session check access in
I went through I couldn't find anything that stood out to me in instead of
I'm not sure how to do this.
I think this is what @lxbsz 's commit is trying to test here, |
Yeah. Agree. [...]
Sage has one commit to add the wanted caps: But didn't mention why. |
... MDS auth caps but don't have CEPHFS_FEATURE_MDS_AUTH_CAPS_CHECK feature bit (i.e., can't check the auth caps sent back to it by the MDS) from establishing a session. Do this in Server::handle_client_session(), and Server::handle_client_reconnect(), where old clients try to reconnect to MDS servers after an upgrade. If the client doesn't have the ability to authorize session access based on the MDS auth caps send back to it by the MDS, then the client may buffer changes locally during open and setattr operations when it's not supposed to, e.g., when enforcing root_squash MDS auth caps. Fixes: https://tracker.ceph.com/issues/56067 Signed-off-by: Ramana Raja <rraja@redhat.com>
|
IMO this approach is nice. And the changes LGTM. |
…6067 A kernel CephFS client with MDS root_squash caps is able to write to a file as non-root user. However, the data written is lost after clearing the kernel client cache, or re-mounting the client. This issue is not observed with a FUSE CephFS client. Signed-off-by: Ramana Raja <rraja@redhat.com>
kclient doesn't have CEPHFS_FEATURE_MDS_AUTH_CAPS required to enforce root_squash. Run root_squash tests only for FUSE client. Signed-off-by: Ramana Raja <rraja@redhat.com>
@batrick @gregsfortytwo would you know why? |
| std::map<std::string, std::string> metadata; | ||
| feature_bitset_t supported_features; | ||
| metric_spec_t metric_spec; | ||
| MDSAuthCaps auth_caps; |
There was a problem hiding this comment.
Writing down a discussion I had with Ramana today:
I do not believe this is the right approach. While it appears to be future-proof because now the Client "knows" its auth caps and can be relied on to implement new authorizations (like root_squash), it has serious issues:
- Old clients will not understand new
MDSAuthCapsversions. So a new client feature flag will be required for every change which requires new authorization handling in the Client. - Clients have to know how to parse these caps. This is trivial for ceph-fuse but not for the kernel client. Parsing grammars in the kernel is something we should avoid.
Because we still need feature bits for any new authorization types, I think this should be handled as new fields (gid/uid/root_squash) in the MClientSession message with corresponding client feature flags. The server tells the client about these authorization restrictions. It's up to the clients to implement them. If the operator wants to enforce them, they can set the required client features [1].
[1] https://docs.ceph.com/en/latest/cephfs/administration/#required-client-features
There was a problem hiding this comment.
Writing down a discussion I had with Ramana today:
I do not believe this is the right approach. While it appears to be future-proof because now the Client "knows" its auth caps and can be relied on to implement new authorizations (like root_squash), it has serious issues:
- Old clients will not understand new
MDSAuthCapsversions. So a new client feature flag will be required for every change which requires new authorization handling in the Client.- Clients have to know how to parse these caps. This is trivial for ceph-fuse but not for the kernel client. Parsing grammars in the kernel is something we should avoid.
Yeah, for kclient it's true.
Because we still need feature bits for any new authorization types, I think this should be handled as new fields (gid/uid/root_squash) in the
MClientSessionmessage with corresponding client feature flags. The server tells the client about these authorization restrictions. It's up to the clients to implement them. If the operator wants to enforce them, they can set the required client features [1].
Let me check how to do with this.
Thanks @batrick
[1] https://docs.ceph.com/en/latest/cephfs/administration/#required-client-features
There was a problem hiding this comment.
Because we still need feature bits for any new authorization types, I think this should be handled as new fields (gid/uid/root_squash) in the
MClientSessionmessage with corresponding client feature flags. The server tells the client about these authorization restrictions. It's up to the clients to implement them. If the operator wants to enforce them, they can set the required client features [1].
I think for something security-focused like this, we can't rely on the operator setting required flags. What we can do is have the MDS refuse to mount a client which doesn't understand cephx caps that involve client-side enforcement. Does that work?
There was a problem hiding this comment.
Because we still need feature bits for any new authorization types, I think this should be handled as new fields (gid/uid/root_squash) in the
MClientSessionmessage with corresponding client feature flags. The server tells the client about these authorization restrictions. It's up to the clients to implement them. If the operator wants to enforce them, they can set the required client features [1].
@batrick, I remember we discussed a bit about the above approach a while back and recall that @lxbsz was intending to use MDSAuthCaps to fix a set of async dirops bugs in the kclient. Are those still valid @lxbsz? FWIW, The alternate approach to this was discussed here - #48027 (comment).
There was a problem hiding this comment.
I think for something security-focused like this, we can't rely on the operator setting required flags. What we can do is have the MDS refuse to mount a client which doesn't understand cephx caps that involve client-side enforcement. Does that work?
We can make it the default for the mons to automatically add these bits to the required client features bitset but that would probably be very surprising to have all existing client mounts get evicted on upgrade.
There was a problem hiding this comment.
Because we still need feature bits for any new authorization types, I think this should be handled as new fields (gid/uid/root_squash) in the
MClientSessionmessage with corresponding client feature flags. The server tells the client about these authorization restrictions. It's up to the clients to implement them. If the operator wants to enforce them, they can set the required client features [1].@batrick, I remember we discussed a bit about the above approach a while back and recall that @lxbsz was intending to use MDSAuthCaps to fix a set of async dirops bugs in the kclient. Are those still valid @lxbsz? FWIW, The alternate approach to this was discussed here - #48027 (comment).
@vshankar Yeah, it should be enough for async case. Currently I think the cephx caps are enough for async. This really could reduce a lot of coding work in kclient.
There was a problem hiding this comment.
Thanks @lxbsz. I'll have a look at the changes this week (trying to clear merge backlog right now).
|
This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved |
|
This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days. |
Author: Ramana Raja rraja@redhat.com
Date: Fri Jul 29 17:51:14 2022 -0400
Author: Ramana Raja rraja@redhat.com
Date: Tue Nov 15 14:00:24 2022 -0500
Author: Ramana Raja rraja@redhat.com
Date: Mon Aug 8 14:33:06 2022 -0400
Author: Ramana Raja rraja@redhat.com
Date: Thu Nov 24 15:48:27 2022 -0500
Contribution Guidelines
To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
Checklist
Show available Jenkins commands
jenkins retest this pleasejenkins test classic perfjenkins test crimson perfjenkins test signedjenkins test make checkjenkins test make check arm64jenkins test submodulesjenkins test dashboardjenkins test dashboard cephadmjenkins test apijenkins test docsjenkins render docsjenkins test ceph-volume alljenkins test ceph-volume toxjenkins test windows