session: Use pidfd for determining ws peer cgroup #21341
Conversation
|
Needs a little more love for Ubuntu 22.04 and RHEL 9, which don't have sys/pidfd.h yet. |
|
Once more with feeling. This is a bit frustrating -- autoconf supports checking for function availability and header availability, but not whether a function is in a particular header. But it turns out that |
martinpitt
force-pushed
the
pidfd
branch
2 times, most recently
from
56fc74b to
47cbfe5
Compare
There was a problem hiding this comment.
I really like the structure you've chosen here: turning "get the dirfd of the /proc entry of the process which opened the other end of this socket, performing whatever checks you can" into a simple API is a really cool approach.
I wonder if you'd consider moving this growing pile of code to a separate file. Maybe that's too much...
Note that kernel 6.13 has a new IOCTL |
|
@bluca Right, thanks! I saw that on LWN somewhere even. I'm happy to put that in once it's in any OS that we support (Debian testing or Fedora 42 once it gets released, presumably). |
There was a problem hiding this comment.
I can't see anything wrong here -- just very minor cosmetic stuff for if you were going to do another round anyway. Maybe @bluca also wants to take a look, though?
We are about to make this code more complex, so let's break up the already long cockpit_session_client_certificate_map_user() function. This is a clean code move, aside from: - renaming `ws_pid_dirfd` to `ws_proc_dirfd` (which is more accurate with that new structure) - fixing two typos in the comment - drop the redundant assert(ucred_len) (already done in the if condition, forgotten to be cleaned up when introducing this code)
We don't expect getsockopt() to write more data than we allocated, that would mess up our stack.
We can get a reliable, PID recycling resistant, /proc query for cockpit-session's ws peer (i.e. the far end of its stdin Unix socket) by getting a pidfd instead of an ucred. This is always the pidfd for the process that started the communication, it cannot be recycled. If the original process does go away, querying the pidfd will just fail, even if a new process with the same pid comes along. We still need to "resolve" the pidfd to a pid to open /proc/pid/cgroup (there is no direct kernel API to get a pidfd's cgroup). But validate the pid *after* that query to ensure it didn't get recycled. This is much easier and safer to do than parsing /proc/pid/stat. However, this requires kernel 6.5, so is not yet available in e.g. Debian 12 or RHEL 9. So keep the pid+time comparison fallback for these older OSes. Thanks to @bluca for the helpful technical advice! cockpit-project#16808 (comment) https://issues.redhat.com/browse/COCKPIT-1207
There was a problem hiding this comment.
Let's get this merged before I find more trivial things to complain about :)
(pending a review from @bluca, if he's willing)
There was a problem hiding this comment.
Looks great to me. Would you mind dropping the @ to me in the last commit message, please? I appreciate the effort to give credit, but the PR description is enough - the issue is that every time the commit gets pushed/backported/cherry-picked I'll get an email from Github, and that's a lot of emails :-)
|
@bluca heh, of course. It'd require another force-push/round of CI though. However, we don't actually do that backporting stuff (on almost everything, except very rare security issues), we just keep updating all OSes to the latest upstream version. So you won't get extra noise. That said, if you still want me to change this, I'll happily do it of course. Thanks again! |
https://github.com/cockpit-project/bots/blob/main/push-rewrite |
No problem, if it's unlikely to be forked around by users then it's ok to leave it, thanks |
We can get a reliable, PID recycling resistant, /proc query for
cockpit-session's ws peer (i.e. the far end of its stdin Unix socket) by
getting a pidfd instead of an ucred. This is always the pidfd for the
process that started the communication, it cannot be recycled. If the
original process does go away, querying the pidfd will just fail, even
if a new process with the same pid comes along.
We still need to "resolve" the pidfd to a pid to open /proc/pid/cgroup
(there is no direct kernel API to get a pidfd's cgroup). But validate
the pid after that query to ensure it didn't get recycled.
This is much easier and safer to do than parsing /proc/pid/stat.
However, this requires kernel 6.5, so is not yet available in e.g.
Debian 12 or RHEL 9. So keep the pid+time comparison fallback for these
older OSes.
Thanks to @bluca for the helpful technical advice!
#16808 (comment)
https://issues.redhat.com/browse/COCKPIT-1207