mds: cephx path restriction incorrectly rejects snapshots of deleted directory#58419
mds: cephx path restriction incorrectly rejects snapshots of deleted directory#58419
Conversation
Signed-off-by: Patrick Donnelly <[email protected]>
Signed-off-by: Patrick Donnelly <[email protected]>
A snapped unlinked directory is moved to the stray directory. Files in that directory will not have correct stray_prior_path set because the snapshot was naturally taken before the file is unlinked. Use the parent directory's stray_prior_path for cephx path access controls. Fixes: https://tracker.ceph.com/issues/66828 Signed-off-by: Patrick Donnelly <[email protected]>
|
jenkins test make check arm64 |
| # should be | ||
| # 2024-07-04T02:11:26.990+0000 7f6b14e71640 20 Session check_access: [inode 0x10000000002 [2,2] ~mds0/stray2/10000000001/file ...] caller_uid=1141 caller_gid=1141 caller_gid_list=[1000,1141] | ||
| # 2024-07-04T02:11:26.990+0000 7f6b14e71640 20 Session check_access stray_prior_path /dir1/dir2 | ||
| # 2024-07-04T02:11:26.990+0000 7f6b14e71640 10 MDSAuthCap is_capable inode(path /dir1/dir2 owner 1141:1141 mode 0100644) by caller 1141:1141 mask 1 new 0:0 cap: MDSAuthCaps[allow rws fsname=cephfs path="/dir1"] |
There was a problem hiding this comment.
What's up with these log-lines-as-comments?
There was a problem hiding this comment.
just documenting what to look for in the logs.
| */ | ||
| if (gpdiri->is_stray()) { | ||
| /* just check access on the parent dir */ | ||
| path = pdiri->get_projected_inode()->stray_prior_path; |
There was a problem hiding this comment.
I don't think I quite understand the claim
Files in that directory will not have correct stray_prior_path set because the snapshot was naturally taken before the file is unlinked.
We still have to unlink the file; why doesn't that set the stray_prior_path correctly?
Just as importantly, are we sure we're correct to grab permissions by iterating up directories like this? Can't we check_access on a directory inode and then go up to its parent and get permission denied, even if had access to the actual directory we were trying to operate on?
There was a problem hiding this comment.
I don't think I quite understand the claim
Files in that directory will not have correct stray_prior_path set because the snapshot was naturally taken before the file is unlinked.
We still have to unlink the file; why doesn't that set the stray_prior_path correctly?
stray_prior_path is part of the inode_t which is projected and set when the inode is unlinked. A snapshot of that inode would not have visibility of that since it's in a future project of the inode. (And we can't use it even if we force looking at it since the inode may have been renamed elsewhere since the snapshot.)
Just as importantly, are we sure we're correct to grab permissions by iterating up directories like this?
Well, we need the stray_prior_path so that's what we have to do. But, it's not completely correct and maybe can't be with existing information, consider:
mkdir a
touch a/file
mkdir .snap/one
mv a b
rm -rf b # a's stray_prior_path is now "/b"
ls .snap/one/a/file # uses stray_prior_path "/b" but should be "/a"
Tracking the stray_prior_path correctly in this situation is difficult since we'd need to record the path changes during rename. I don't think it's worth the trouble when we're talking about access control on a read-only snapshot.
Can't we check_access on a directory inode and then go up to its parent and get permission denied, even if had access to the actual directory we were trying to operate on?
No. Because this change only affects access control to non-directories. An unlinked directory inode's parent is always a stray directory. That is handled on line 1055 above.
|
This PR is under test in https://tracker.ceph.com/issues/67516. |
|
This PR is under test in https://tracker.ceph.com/issues/67563. |
|
Hello, is this ready to be merged or is there still work to be done? Thank you |
It's under testing by Patrick. Once the fs suite run is reviewed for failures, the change will get merged. |
|
Thank you! |
* refs/pull/58419/head: mds: generate correct path for unlinked snapped files qa: add test for cephx path check on unlinked snapped dir tree mds: add debugging for stray_prior_path Reviewed-by: Milind Changire <[email protected]>
5 participants
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. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an
xbetween the brackets:[x]. Spaces and capitalization matter when checking off items this way.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 windowsjenkins test rook e2e