Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fsinfo: include access bits for cockpit-files #21596

Open
jelly opened this issue Feb 7, 2025 · 6 comments · May be fixed by #21603
Open

fsinfo: include access bits for cockpit-files #21596

jelly opened this issue Feb 7, 2025 · 6 comments · May be fixed by #21603

Comments

@jelly
Copy link
Member

jelly commented Feb 7, 2025

In cockpit-files in some places we now check for file read/write or directory read/write with test -w, however this is something fsinfo could provide!

When watching a directory fsinfo already reacts on changes to files and the directory itself when permissions change except for ACL's. (Calling setfacl on a file gives no change event).

It seems we do watch for ATTRIB changes but they aren't send as updates.

We could provide if a file is readable / writeable and also for cwdInfo (the watched directory) with:

{
  'writeable': os.access(filename, os.W_OK)
  'readable': os.access(filename, os.R_OK)
}

Sadly os.access returns a boolean and not a bitmask so we would need 2 extra syscalls per file, unless I am missing something. I have tested ACL's and os.access takes those into account.

ACL Demo

# as root
echo 'phone' > /tmp/banana
chmod 640 /tmp/banana

# as user
$ cat /tmp/banana
permissions error

# as root
setfactl -m u:admin:r /tmp/banana

# as user
cat /tmp/banana
phone

As the access man page mentions something about ACL's not working on older glib'cs I tested on rhel-8-10 and there ACL's are accounted for.

@martinpitt
Copy link
Member

Sadly os.access returns a boolean and not a bitmask so we would need 2 extra syscalls per file, unless I am missing something

This would be opt-in like everything else, in the info attribute, no? If you really would ask for both readable and writable, it would be two syscalls. But (1) that seems rare, and (2) unavoidable. os.access() cannot be derived from looking at the permission, as there's so much more going into it (user, auxiliary groups, ACLs, SELinux, etc.)

So this seems fine to me.

@allisonkarlitskaya
Copy link
Member

I think this makes sense, subject to:

  • it absolutely must be conditional (because of the syscall overhead)
  • I prefer the names r-ok and w-ok, and maybe also add x-ok while you're at it
  • please do it via the already-open fd (faccessat(fd, "", AT_EMPTY_PATH)) instead of via the filename

@allisonkarlitskaya
Copy link
Member

One more: please make sure to use AT_SYMLINK_NOFOLLOW. We don't want to accidentally report information about symlink targets.

@allisonkarlitskaya
Copy link
Member

It doesn't look like Python passes AT_EMPTY_PATH and it doesn't look like there's a flag to get us there either, so probably you have to do this name-based using the dir_fd. That's not excellent, but it's already what we do for stat, so eh...

@jelly
Copy link
Member Author

jelly commented Feb 10, 2025

Just checked the Python bug tracker and there is no issue yet for supporting passing AT_EMPTY_PATH to faccessat at all and it would need to support a new flags argument for that.

jelly added a commit to jelly/cockpit that referenced this issue Feb 10, 2025
For cockpit-files it is useful to know if the current watched directory
or for example a text file is editable for the current user. Doing this
based on the existing file permissions doesn't take ACL's into account.

The `access` syscall only handles one access check (read/write/execute)
per call making it rather inefficient to check for multiple scenario's,
so that's why there are separate attrs depending on what the user want
so in worst case we only add 1 extra syscall.

As Python does not support AT_EMPTY_PATH there is a workaround to read
the file from /proc/self this is only required for reading the access
bits of the current watched directory.

Closes: cockpit-project#21596
@jelly jelly linked a pull request Feb 10, 2025 that will close this issue
@jelly
Copy link
Member Author

jelly commented Feb 10, 2025

Just checked the Python bug tracker and there is no issue yet for supporting passing AT_EMPTY_PATH to faccessat at all and it would need to support a new flags argument for that.

Honestly, this is not the worst:

AT_EMPTY_PATH = 0x1000
if libc.faccessat(dir_fd, "", os.R_OK, AT_EMPTY_PATH) == 0:
    print('can read dir')

Also note that AT_EMPTY_PATH is Linux specific so unsure if Python wants to support that /o\

jelly added a commit to jelly/cockpit that referenced this issue Feb 13, 2025
For cockpit-files it is useful to know if the current watched directory
or for example a text file is editable for the current user. Doing this
based on the existing file permissions doesn't take ACL's into account.

The `access` syscall only handles one access check (read/write/execute)
per call making it rather inefficient to check for multiple scenario's,
so that's why there are separate attrs depending on what the user want
so in worst case we only add 1 extra syscall.

As Python does not support AT_EMPTY_PATH there is a workaround to read
the file from /proc/self this is only required for reading the access
bits of the current watched directory.

Closes: cockpit-project#21596
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants