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

polkit: create hook to check for polkit permissions #20283

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

chriswiggins
Copy link
Contributor

This is a draft PR to understand whether or not something like this would be feasible to bring more polkit-based checks into Cockpit.

We have a use-case whereby we don't want to give a user sudo access, but we do want them to manage things such as the network interfaces.

This hook uses pkcheck to test permissions and return an array of booleans that the user can use to determine if permissions are allowed or not. This currently checks a wide-range of NetworkManager permissions as a catch-all however it can definitely be refined in the future, should the approach be acceptable.

My thinking is that I'd like to move the pkcheck exec across to the polkit DBus interface, but baby steps first 😄

Thanks for any feedback

@chriswiggins
Copy link
Contributor Author

Further to the above, I'd also like to look into creating a hook to get really fine-grained permissions for the systemd services page too, so we can selectively check if a user is allowed to start/stop a service and enable/disable etc

Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @chriswiggins for taking a stab at this! This has been on our wishlists for quite some time (#11033).

Let's ponder the design a bit first.

pkg/lib/hooks.js Outdated Show resolved Hide resolved
pkg/lib/hooks.js Show resolved Hide resolved
Comment on lines 120 to 132
<NetworkPage privileged={superuser.allowed}
<NetworkPage privileged={superuser.allowed || polkitAllowed}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I mean -- if you use superuser: try, you don't have to explicitly check superuser any more.

pkg/lib/hooks.js Outdated Show resolved Hide resolved
@chriswiggins
Copy link
Contributor Author

Hey @martinpitt - I've made those changes you mentioned and am much happier with how it looks - thanks for the pointers!

Let me know what you think, I'd be keen as part of this PR to add more polkit checks in around the place as there are more things we'd like to allow too

@chriswiggins
Copy link
Contributor Author

Hey @martinpitt - more of an update as I have been trying to do the same thing in the systemd services page.

Turns out we can't ask specifically if a user is allowed to say org.freedesktop.systemd1.manage-units on a particular unit from a user session like so:

$ pkcheck --action-id org.freedesktop.systemd1.manage-units -p $$,0,1000 -d unit cockpit.service
Error checking for authorization org.freedesktop.systemd1.manage-units: GDBus.Error:org.freedesktop.PolicyKit1.Error.NotAuthorized: Only trusted callers (e.g. uid 0 or an action owner) can use CheckAuthorization() and pass details

This is not due to anything other than PolicyKit requiring the user who is running the process to be the one that performs the check (as far as I can tell, that pkcheck succeeds as root) - unfortunately the PolicyKit docs are sparse 😅

This works on the NetworkManager page I think because we aren't asking for further detail

On Yocto cockpit is running as root, but Ubuntu I see it running as a cockpit-ws user. Is there anything typically running by cockpit that is running with uid 0? The reasoning behind this is to have a service that we can communicate with to do the policykit checks. Any pointers or hidden knowledge about how systemd/polkit works so we don't need to be doing the auth check as root?

@martinpitt
Copy link
Member

@chriswiggins sorry for the late reply! I've been swamped last week with other stuff.

I suppose it makes sense to restrict the CheckAuthorization(), to avoid allowing unpriv users to brute-force through all possible actions/combinations? Kind of like normal users can't read /etc/sudoers? But anyway, that's what we have to live with then. I just tried this on my Fedora system, and it behaves the same way:

❱❱❱ pkcheck --action-id org.freedesktop.systemd1.manage-units -p $$,0,1000 -d unit cockpit.service
Error checking for authorization org.freedesktop.systemd1.manage-units: GDBus.Error:org.freedesktop.PolicyKit1.Error.NotAuthorized: Only trusted callers (e.g. uid 0 or an action owner) can use CheckAuthorization() and pass details

❱❱❱ pkcheck --action-id org.freedesktop.systemd1.manage-units -p $$
polkit\56retains_authorization_after_challenge=1
Authorization requires authentication and -u wasn't passed.

❱❱❱ pkcheck --action-id org.freedesktop.systemd1.manage-units -p $$ -u
polkit\56retains_authorization_after_challenge=1
Authorization requires authentication but no agent is available.

and indeed I just saw that in Cockpit's firewall page we do this wrong:

pkg/networkmanager/firewall-client.js:cockpit.spawn(['sh', '-c', 'pkcheck --action-id org.fedoraproject.FirewallD1.all --process $$ --allow-user-interaction 2>&1'], { superuser: "try" })

I think we need to take a look at this again -- if it merely requires user interaction (authentication as that user) without having that privilege, or general sudo privs, it's fine. If it fails wiht one of the above errors if you dont', then it's not worth doing this in the first place -- if we already can get root through sudo/pkexec, then we don't need the fine-grained permissions.

Is there anything typically running by cockpit that is running with uid 0?

Sort of.. but irrelevant. You can't just ask for another process:

❱❱❱ pkcheck --action-id org.freedesktop.systemd1.manage-units -p 1
Error checking for authorization org.freedesktop.systemd1.manage-units: GDBus.Error:org.freedesktop.PolicyKit1.Error.NotAuthorized: Only trusted callers (e.g. uid 0 or an action owner) can use CheckAuthorization() for subjects belonging to other identities

if that worked, it'd be rather pointless 😁 So what matters here is the process which does that pkcheck call, which is the bridge -- and that runs as the user. (And by definition there's no root bridge).

Thanks for pointing this out! This requires some more research whether even the existing firewalld code makes actual sense?

@chriswiggins
Copy link
Contributor Author

Yes a very tricky one indeed!

What about we still add the ability to check for "high-level" polkit permissions for units (like I did in NetworkManager) and then the more fine-grained stuff can just fail if you don't have permission on that particular unit?

I have an idea in my head - might draft some changes up tomorrow to show what I mean. Does Cockpit handle DBus auth requests like it handles sudo by popping a window up?

@chriswiggins
Copy link
Contributor Author

Hey @martinpitt - looping back into this after a small hiatus! Would there be appetite for a very small helper utility that ran as root, which could run the pkcheck and then report back the result to the bridge? I'm thinking it is something that users could optionally include?

What is the best way to work out a design for this?

@martinpitt
Copy link
Member

@chriswiggins Aside from suid helpers being a pain in the back to develop and maintain, I'm afraid I have to reject the mere idea. Tools like polkit or sudo are intentionally restrict what unprivileged users can find out about someone's privileges, as that would allow users to silently (no "not permitted" log entries) probe for possible attack vectors.

If it's safe (and efficient) to enumerate all allowable PK actions for a user, this change should happen in polkit itself in my opinion.

Adding such a suid root helper which is world-executable (so that you can call it in user sessions) circumvents these restrictions. Even if you route it through the web server and only make it executable by cockpit-wsinstance, normal users could still call that API with normal user privileges, so it's just one more indirection to circumvention.

So I think the only possible way would be to put the UI into "fake admin" mode, expose all controls, and lots of them would fail with permission errors -- i.e. the user has to know what they are allowed to do. Not exactly a great experience, although it may help for some use cases.

This is a hard problem, sorry 😢

@chriswiggins
Copy link
Contributor Author

I agree it's a hard problem! A fun one to dive into 😉

How about a dbus helper that we can call with something like canPerformAction('modify_network') which behind the scenes has a map of the various PolKit actions that could allow the user to update network settings? That way, it is possible to potentially show an allowed state (rather than all or nothing), and if the specific action they're trying to perform isn't allowed, they still get asked for a password? Or is that still going down the security-by-obscurity route?

I.e as an example:

import subprocess
from pydbus import SystemBus
from gi.repository import GLib

class CockpitPermissionsHelper:
    """
    D-Bus service to handle high-level permission checks.
    """
    def CanPerformAction(self, action, user_pid):
        # Map high-level action to specific polkit action
        ACTION_MAP = {
            "modify_network": [
                "org.freedesktop.NetworkManager.reload",
                "org.freedesktop.NetworkManager.checkpoint-rollback",
                "org.freedesktop.NetworkManager.network-control",
                "org.freedesktop.NetworkManager.settings.modify.global-dns",
                "org.freedesktop.NetworkManager.settings.modify.hostname",
                "org.freedesktop.NetworkManager.settings.modify.own",
                "org.freedesktop.NetworkManager.settings.modify.system"
            ]
        }
        
        polkit_actions = ACTION_MAP.get(action)
        if not polkit_actions:
            return "Invalid action"

        # Call pkcheck to check if the user is allowed to perform the action
        allowed = False
        for polkit_action in polkit_actions:
          result = subprocess.run(['pkcheck', '--action-id', polkit_action, '--process', str(user_pid)], capture_output=True, text=True)
          if result.returncode == 0:
              allowed = True
              break
          
        return "yes" if allowed else "no"

def main():
    # Connect to the system bus and export the service
    bus = SystemBus()
    bus.publish("org.cockpit.PermissionsHelper", CockpitPermissionsHelper())

    # Run the main loop
    loop = GLib.MainLoop()
    loop.run()

if __name__ == "__main__":
    main()

@martinpitt
Copy link
Member

@chriswiggins I feel like this quickly dives into a rabbit hole of an unmaintainable web of pk mappings, together with a combinatorial explosion of new tests, while it still undermines polkit's query restrictions 😢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants