canonical / pebble

Take control of your internal daemons!
https://canonical-pebble.readthedocs-hosted.com/
GNU General Public License v3.0
143 stars 54 forks source link

feat(state,daemon,client): add user-specific notices #323

Closed olivercalder closed 8 months ago

olivercalder commented 10 months ago

We wish to be able to restrict the visibility of a given notice to particular users. This PR adds to Notice a userID field which corresponds to the UID of the intended recipient of the notice (usually its creator). The userID is stored as a *uint32, where nil indicates that the notice is public, and thus accessible to all users.

The NoticesFilter includes a UserID *uint32 as well, and if it is not nil, then only public notices and notices matching the filter UserID are returned. Public notices are always returned regardless of the UserID filter, so long as they match the other filters as well.

The root/admin users have access to all notices, while other users may only view their own notices or public notices. This is enforced by the API, which by default populates the UserID filter with the request UID. The state is not aware of the permissions of admin or non-admin users, and treats all non-nil UIDs identically.

Instead of their own notices, admin users may get the notices of another user by passing the user-id=<uid> argument to the API at /v1/notices. To get all notices for all users, an admin may pass select=all instead, though they cannot use both user-id and select in the same request.

When creating a notice via the API, the request UID is used as the userID of the notice. There is no way to create a public notice via the API.

The client and CLI correspond closely to the API. The only distinction is that when using the pebble notice TYPE KEY variant, there is now an optional --uid UID argument (admin only) which lets an admin look up by type and key a particular notice from another user.

benhoyt commented 10 months ago

Thanks @olivercalder! I'll try to take a look at this PR over the sprint weeks. Maybe we can even chat about it in person if there's need. :-)

olivercalder commented 9 months ago

This looks good to me now. I'd like to run the final design past @niemeyer tomorrow at our APAC spec review meeting, and if no concerns come out of that, I'll get this merged.

Excellent, thank you! Please let me know if there's anything that needs adjustment after the sync. I am off almost all of this week, but I'm happy to make tweaks if needed. Also, @pedronis said before he'd be interested in reviewing the PR before it is merged -- he is off until Wednesday -- so perhaps if possible it would be preferable to wait until he can take a look as well. But he will likely have a lot of reviewing to catch up on, and if @neimeyer approves I think we're probably fine to move forward nonetheless.

olivercalder commented 9 months ago

I am likely going to implement the new user-specific notices spec in a new PR, so closing this one for now.

olivercalder commented 9 months ago

I've reimplemented per-user notices in the state, daemon, and client according to the updated spec. I have a few questions about the CLI, so I haven't pushed my changes for that yet. I'd appreciate feedback on a few points, notably:

  1. If we don't have a user ID field in the pebble notice command, there may be multiple notices with the same type/key but different user IDs. What should we do if there are multiple? Should another argument be added for user ID?
  2. A few of the arguments for the cli commands are admin-only. Do we want to/is there a way to hide them from the usage strings for non-admin users?

Edit: I did push my changes for the pebble notice cli, as the previous iteration was incompatible with the new changes to the client etc.

anpep commented 9 months ago
  1. If we don't have a user ID field in the pebble notice command, there may be multiple notices with the same type/key but different user IDs. What should we do if there are multiple? Should another argument be added for user ID?

As far as I know, when using a local unix socket for the client connection, the Pebble daemon will get the UID/GID of the process sending data through the unix socket (see internals/daemon/ucrednet.go). My intuition tells me we should use this value to filter out notices in the daemon. If the UID is 0, my proposal would be to provide an -a/--all option so that we can toggle between sending out only private notices or all notices regardless of the UID. I wonder what @benhoyt thoughts are on this (:

@benhoyt Incidentally, this gives us some food for thought on how are we going to handle this with the future interface-based approach to authentication in Pebble, and how we are going to map TCP connections to UIDs/GIDs.

benhoyt commented 9 months ago

Excellent, thanks @olivercalder. I'll review this in the next day or two. Can you also please make the relevant updates to the PR description?

benhoyt commented 8 months ago

Oh also, please update the PR description once again. :-)