Implement IsClientAuthorized() for FreeBSD and stop failing open #209
Conversation
The current version fails open if we don't have some analog for the Linux SO_PEERCRED functionality implemented for the platform we're building on. This is incredibly surprising if polkit has been specifically requested, as the default implementation will just allow all access without consulting polkit at all.
FreeBSD's analog to SO_PEERCRED is LOCAL_PEERCRED, which returns a (ABI stable) `struct xucred` rather than a `struct cred`. Paper over the platform differences with a typedef and a couple macros.
|
CC @arrowd as our port maintainer |
|
This looks good to me, thanks for fixing this! |
|
|
||
| #include <polkit/polkit.h> | ||
| #include <stdbool.h> | ||
|
|
||
| #ifdef __FreeBSD__ |
There was a problem hiding this comment.
I don't know much about socket authentication, but what about macOS? Is the struct xucred only used on FreeBSD? A quick Google search implies that it's also used on Mac. Would you mind explaining what ucred vs xucred is about?
There was a problem hiding this comment.
xucred is the ABI-stable userland export version (subset) of ucred -- if you have the former then the latter is considered kernel-only and it will populate an xucred for things in userland that want it.
There was a problem hiding this comment.
macOS also seems to have LOCAL_PEERCRED, but I don't know what the polkit-on-macOS situation is like so I haven't bothered trying to expand it in that direction. Likely it also wants the xucred version of this.
| #elif defined(HAVE_POLKIT) | ||
|
|
||
| #error polkit is enabled, but no socket cred implementation for this platform | ||
|
|
||
| #else | ||
|
|
||
| unsigned IsClientAuthorized(int socket, const char* action, const char* reader) |
There was a problem hiding this comment.
Given the above, #error polkit is enabled, but no socket cred implementation for this platform, I'm not sure if this will matter or not. But I wonder if this function still has an effect, if we can log some kind of message to stderr stating that the allow-all IsClientAuthorized() function is being used.
May be outside the scope of this PR, but wanted to suggest it as a way to make it more obvious if others run into this.
There was a problem hiding this comment.
The allow-all one is what you should get when you build with -Dpolkit=false to avoid sprinkling more HAVE_POLKIT around and allow other authorization mechanisms to be plugged in instead. Its use is actually expected rather than an error in the default configuration, and that seems more or less fine.
We can provide a sensible polkit implementation on FreeBSD with our analog to SO_PEERCRED, so do it. The functionality is identical modulo some naming differences in the xucred that we get back.
While we're here, don't fail open if polkit was specifically requested. AFAICT the meson build won't enable it without an explicit -Dpolkit=true, so if we got to auth.c with HAVE_POLKIT then the build requested it. Failing open (i.e. not consulting the policy) results in a daemon that behaves quite surprisingly when the user expected it to be consulting polkit before granting access. Considering the nature of the daemon, this seems like a safer/better default.