User admin #2212
User admin #2212
Conversation
|
There's a lot here, but I'd like to focus on the API endpoints first since those will be permanent and I'd like to get this deployed locally soonish. |
|
|
jeffmahoney
force-pushed
the
user-admin
branch
from
71b05c6
to
21fb07f
Compare
|
(No real changes with the re-push. There was just some oddities with generated pb files where they shouldn't be.) |
api/users.go
Outdated
| if !perm || err != nil { | ||
| return nil, status.Error(codes.PermissionDenied, | ||
| "User is not allowed to enumerate users.") | ||
| } | ||
|
|
||
| filterOrgs := true | ||
|
|
||
| if isMemberOfRootOrg(user_record.Orgs) { |
There was a problem hiding this comment.
There is an assumption here that a member of the root org will have admin access to all the other orgs which is not currently the case. It is maybe a reasonable assumption in practice though.
I think this function should replicate (or even refactor) the VQL equivalent here
https://github.com/Velocidex/velociraptor/blob/master/vql/server/users/users.go#L77
Generally we should have once source of truth in the code and call it both from the VQL and API if we are extending the API.
There was a problem hiding this comment.
I made an assumption early on that the root org was intended to be hierarchically higher than the other orgs, and now I see that assumption is invalid.
There was a problem hiding this comment.
Kind of - for convenience an admin of the root org automatically gets the org admin permission as well
https://github.com/Velocidex/velociraptor/blob/master/acls/roles.go#L101
so we dont need a special role just for org admins but it is always possible to give users individual permissions and that permission is what actually matters
api/users.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func grantUserRoles(config_obj *config_proto.Config, org_manager *services.OrgManager, |
There was a problem hiding this comment.
Should we just call the VQL function https://github.com/Velocidex/velociraptor/blob/master/vql/server/users/grant.go#L23
It makes it much easier to test and we then know the logic is the same as what is tested.
api/users.go
Outdated
| return nil, err | ||
| } | ||
|
|
||
| if !org_admin && checkOrgMembership(user_record.Orgs, active_user.Orgs) == false { |
There was a problem hiding this comment.
You generally dont need to check org membership because the ACL exists in each org anyway so the check above ensures the user is an admin in the org.
There was a problem hiding this comment.
I think this ends up being a fundamental issue with this entire PR. I wrote most of this against 0.6.4 before the organizations were introduced and had to update it against 0.6.6 (and now 0.6.7-rc1) to submit it. The user admin UI is not taking into account the current organization of the principal. It's displaying and operating in a global view, but constraining it by the list of orgs the principal is a member of and, when appropriate, an intersection of the orgs of the user being updated/deleted. I was basing that on VelociraptorUser having a list of orgs and a set of roles and mistakenly assuming that meant that the roles applied to every Org, when it probably means that the roles correspond to the current org.
I'll need to go back and rework this.
There was a problem hiding this comment.
All you really need to do is to delegate to the VQL functions to do the actual work - the API should be a thin wrapper around the VQL which is supposed to do the right thing in all cases
There was a problem hiding this comment.
I suppose I don't have a clear grasp on what the roles mean in the scope of orgs and where the separation of the org takes effect.
I see that ORG_ADMIN in the current org allows:
- COLLECT_CLIENT permission in any org via
v1/CreateHuntandhunt_add() - Listing users across orgs
- Creating new orgs
SERVER_ADMIN is typically evaluated against the current org, but often the operations the SERVER_ADMIN role can perform aren't limited to that org. For example, user_grant() checks for SERVER_ADMIN in the current org but then allows the caller to operate on roles in any org. user_grant() can also be used to modify the ACLs in each org separately so that having an ACL in one org doesn't mean it exists in other.
I suppose my code was trying to refine this a bit, at least for the API, so that SERVER_ADMIN only applies to the orgs to which the user belongs. ORG_ADMIN allows SERVER_ADMIN to be global, but the the direct implication of SERVER_ADMIN -> ORG_ADMIN means that distinction doesn't exist.
Do orgs only exist for the lesser privileged roles and SERVER_ADMIN is intended to be global? If that's the case, we should probably avoid inconsistencies with SERVER_ADMIN only being set on some orgs.
There was a problem hiding this comment.
ORG_ADMIN always allows full org control. I guess it is an oversight that a SERVER_ADMIN can grant ORG_ADMIN to a user without having ORG_ADMIN themselves.
This new permission model is pretty new so we do need to think through some of these implications. This is why it is important to have consistent calls into the same well tested set of functions instead of reimplement the logic in slightly different ways in different places.
If you dont mind I can help with the PR by refactoring the VQL functions to make them easier to call from the API functions. Otherwise this is really a great addition ! Thanks for kicking the tires on the permission system too, clearly we have some way to go on it :-).
The whole point of the multi org feature is to make it safe for an org admin to provision an org and give full admin rights (SERVER_ADMIN) to a user in that org without escalation to other orgs. One potential escalation path is arbitrary file write, execve and other dangerous plugins which #2214 is enforcing so we are making progress towards that goal - but we are not there yet!
There was a problem hiding this comment.
I've already started in on refactoring the VQL stuff for user manipulation. The general idea is to factor out fetching the ACLManager from VQL so that both the API and VQL callers can pass it in to a common implementation under services/users. I'm using a request struct for the refactor that gets replaced by the api_proto version later, so that there's no real difference on the VQL side and a straight line on the API side. In doing so I found a bit of a weakness in my original approach: you have to essentially specify the entire user with all the fields every time and when the gRPC http proxy ends up removing booleans if they're false or unset, they also always need to be specified. Not ideal. So now I'm using two maps of string->string and string->bool to indicate new values for fields.
api/users.go
Outdated
| in *api_proto.UserRequest) (*emptypb.Empty, error) { | ||
|
|
||
| users_manager := services.GetUserManager() | ||
| active_user, org_config_obj, err := users_manager.GetUserFromContext(ctx) |
There was a problem hiding this comment.
More standard to call this "principal" is the user that is initiating the activity
|
Do you think it needs to go in the sidebar? The screen is only useful for admins and only sometimes (user management is not very common). We should try to keep the sidebar uncluttered as much as possible. Maybe add a link from the welcome screen? Or maybe in the user preferences/change password screen? Otherwise we can condition the sidebar entry on the user being an org admin. |
|
You're right. An earlier version had a version of GetUser from a colleague which just used READ_RESULTS so
You're right. An earlier version had a version of GetUser from a colleague which just used READ_RESULTS so that would at least show the user their own record. Once I realized that was inconsistent with what VQL offered, I fixed it but never updated the sidebar icon. It should be conditional. |
|
I noticed that protoc creates a bunch of churn in the repo and so pushed an update to the make_protobuf.sh - you should be able to resync and regenerate to clean up the proto modifications |
Thanks, I was wondering if there was a better way to do that. |
|
I have just submitted a bunch of user management functions that make it easier and safer to add/remove from orgs etc. Looking at the UI in this PR I think it does not reflect the permission model in Velociraptor very well maybe we can rethink the layout ? The model in Velociraptor is:
Common actions we need to perform with users:
I dont think this UI reflects this model:
Maybe a tree view is better? Have the top level being the org, the under each org we see the users, then one can click on each user to see their roles within the org. Alternatively maybe a cascading table with two levels? Left side table is orgs when user selects the org, next table populates with users in the org, then when a user is selected we show the roles etc. Maybe then have a toolbar or something to add the user to other orgs (with a required role) so then the same user shows up under different orgs? |
|
I think most of what I have in the UI now can be repurposed to work with orgs properly. I think I would represent the users by separating the user list on the right into three areas: users belonging to the active org, next users belonging to orgs other than the current one but in which the user is an admin and can still see full user records, and last users belonging to other orgs entirely. While looking through the code, I think there is still some ambiguity as to how the org admin role is supposed to work. In some uses it's only checked against the root org but in others it's only checked against the current org. Is the org admin role intended to be global or is it intended to have the ability to add and remove users only in the orgs where the user holds that role? |
|
The org admin role is basically a bootstrap mechanism to allow orgs to be created in the first place. Once orgs have their own admin the intention is that they should be able to manage all their own users. I have not thought too much about the implications of making a user have org admin in the org rather than the root org - it might only make sense to have org admin in the root org anyway. Currently the permission is provided by the "administrator" role only in the root org. We probably should also stop an org admin from giving the permission on the org to another user too. Note that the list of users is actually filtered now to only those users that the principal has visibility of (i.e. if they have a reader permission in the org they can only see their name, admin perm in any of their org will see their full details). Normal use case is that an admin has one or two orgs and some users already in these orgs. They might need to move users between the orgs or create new users but have no ability to bring other users into the org at all (they can not see them even). So the third category in your GUI will always be empty. |
|
Ok, sure, the UI won't have it because ListUsers won't, but that's the only reason. Both the API and the VQL |
|
I was actually debating this when writing the code - you are right that the AddUserToOrg API will allow the user to be added if the caller knows their name first and the user already exists. In a sense this is information leakage as the org admin can tell if a user already exists or not. If they try to add the user to the org and get a User not exist then they need to create the user first, then retry to add. It also makes the AddUser() code a bit more tricky. The alternative is to automatically create an empty user if the user does not exist so it is seemless but this could lead to a lot of broken user records if people accidentally misspell a name etc will create many empty users. In the end I decided not worry too much about the info leakage because any reasonable UI will present a list of users and then allow to add them so it is almost guaranteed that the user that is being added actually exists (because it just was listed). A UI should present an option to "add new user" and will then call AddUserToOrg with AddNewUser option then immediately call SetPassword on the new account if required. So I think this is just not going to be an issue in practice. |
|
As I'm figuring out the API for updating users correctly using orgs as first-class citizens in the system, I started wondering if the ORG_ADMIN role needs to be refined some more or dropped.
Is there a use case for a user to have SERVER_ADMIN in the root org but not ORG_ADMIN? If the answers to both of these are "no" we should drop ORG_ADMIN entirely. If there is a possible "yes" then we need to determine what the distinction is and not automatically imply ORG_ADMIN from SERVER_ADMIN in the root org. To a certain extent, I think orgs should either be "off" and there's only the root org or "on" and the root org only functions to provide a context outside of the orgs that can be used to manage them. In the latter case, the privileges beyond ORG_ADMIN/SERVER_ADMIN aren't relevant anymore. |
|
Here's what I've got now in terms of UI. I need to split up the changes and stuff them back into easily digestible commits, but it works. The switches select orgs and once selected, the appear in the accordion. Each org title is a link that displays the roles for that org as well as the current effective permissions. The top of the user select pane will always contain the active current org. If the user has access to other orgs, they'll be displayed below. Likewise with org admins.
|
Corrent - the idea is that ORG_ADMIN is like a super user for the entire deployment and is used to bootstrap individual orgs.
This is very easy to do since there is no logical check in user_delete() to make sure there are any admins left over - currently this allows deleting the only admin user for the org leaving the org orphaned :-). It is also possible to remove ORG_ADMIN in the same way which requires re-adding the user from the command line. I was debating if we should build some foot/gun protections here but at least the ORG_ADMIN can fix things if they happen.
An important clarification here is that ORG_ADMIN is a permission, like SERVER_ADMIN is a permission. A role can contain multiple permissions - so the "administrator" role contains both permissions. For clarity it is always better to have clearly defined permissions - even if they typically go together or in practice one implies the other. The ORG_ADMIN and SERVER_ADMIN permissions are very different actions I think so it is still worth separating these out. However, I do acknowledge that they typically go together (at least for the root org) and so the "administrator" role can provide both automatically.
We don't imply these permissions are the same - we just provide a convenient role which grants both these permissions. It is possible to not have that permission at all. So in theory it is possible to have an admin for the root org which does not have ORG_ADMIN permission at all.
I'm not sure I understand this statement :-). Are you suggesting that "administrator" role should not provide ORG_ADMIN by default? so going from a single org deployment to a multi org one requires first granting an additional role? |
This looks cool but I wonder how it will scale to many users and many orgs. I wonder if a tree style hierarchy is more understandable here? I mean in this screenshot the third column from the left is already overflowing and there are only 2 users in 1 org. Perhaps we need two types of UI - to view the roles/permissions we use a simple compressed widget (so in this case it just shows "Read Only User" and does not show all the other "off" roles) and then a GUI that allows to update roles/permissions with a more detailed view of all permissions (maybe in a modal?) From a task based approach the user might want to do the following workflows:
The first requires drilling down: first all the orgs, then selecting the org, all the users in the org, then selecting the user all the roles for that user. It is almost as though we need two separate GUIs:
Then have the user switch between them (e.g. with tabs) - view by user, view by org |
Right, and that's the distinction I'm trying to draw. In theory it's possible to separate them but we're not. If it's defined as not having to be implied, and that it's just an implementation detail, I can deal with that.
I'm saying that when there is a single org, the root org is not special. It just is the only org. When there are multiple orgs, and we keep ORG_ADMIN in the root org, it makes the root org special in terms of how orgs and accounts are managed. Yet it still remains a regular org that interacts with clients/etc in the same way as other orgs. I'm saying we should codify that such that the root org in a multi-org environment is special and the only privileges that can be granted are SERVER_ADMIN and ORG_ADMIN. Perhaps when the second org is created, the first should be converted to a new one as well. but not in how it interacts with clients/etc. It stops being a "peer" org and ends up being kind of a "global" org since a user with ORG_ADMIN must be in the root. Given that distinction, I think it would make sense to define the root org as special in a multi-org environment and to either define the system as being multi-org from the beginning or to migrate the root org to a "new" org when the second org is created. |
jeffmahoney
force-pushed
the
user-admin
branch
from
e5a925f
to
c75c0a6
Compare
Every test case calls self.makeUsers but nothing checks to ensure that the initial state is correct.
The TestDeleteUser testcase takes a user which is a member of one org, adds it to another, removes it from the first, and checks to see if it still exists and remains in the second org. It never checks to see if the membership in the first org was actually completed.
There are no built-in consumers of v1/GetUsers yet, but not having to sort the users on the client makes sense.
To build the user admin GUI, we'll need all of the details for a given user. This endpoint will return permission denied if a user requests the record for another user unless the caller has the SERVER_ADMIN role and ORG_ADMIN for at least one of the orgs listed in the record.
The user admin GUI needs the ability to create and edit a user. These endpoints will accept a POST request to create a user or edit an existing one, It requires the SERVER_ADMIN role to use and membership in any organizations the request contains (or ORG_ADMIN role).
The v1/DeleteUser endpoint accepts a DELETE request to remove the user. It requires the SERVER_ADMIN role. The interaction with orgs needs refinement.
This is the same as v1/GetUsers but shows users from every visible org.
These endpoints will allow the admin to get and set roles on a subset of orgs.
The v1/DeleteUser api endpoint uses the DELETE method, so we'll need support for it in the gui code.
This commit implements the GUI portion of the user admin tool. It allows users with the SERVER_ADMIN role to modify the roles, email, etc of users within their organization. If the user also has the ORG_ADMIN role, it will allow them to modify any user. In addition to the role selection, the UI will show what collection of permissions a particular combination of roles will grant, using tooltips to explain in more detail. The user list is searchable by username and can be limited by role. The organization assignment card will show only the organizations the active user has permission to modify. If no organizations are selected, a Snackbar will pop up explaining that membership in no organizations is the same as membership in the root (or all) organizations. In addition to being able to edit users, users may be created using the same user interface. Passwords may be set and changed if the server is set up to use basic authentication. Otherwise, the password will be randomized unless explicitly set.
Now users can be searched by org or by user
Added warning dialog when about to remove user from org.
|
I have reworked the GUI to work more along the most common workflow:
We need to add a username/password reset section still. |
Introduce a new user management UI that allows admins to manipulate user's org memberships, reset passwords, create new users and change user's roles and permissions. Special thanks to @jeffmahoney for working on this really large feature!
* [Snyk] Upgrade ace-builds from 1.12.3 to 1.12.4 (#2264) fix: upgrade ace-builds from 1.12.3 to 1.12.4 Snyk has created this PR to upgrade ace-builds from 1.12.3 to 1.12.4. See this package in npm: https://www.npmjs.com/package/ace-builds See this project in Snyk: https://app.snyk.io/org/scudette/project/76f4d127-566b-42ef-86f4-bdcbc92b90b4?utm_source=github&utm_medium=referral&page=upgrade-pr * Bugfix: http_client was unable to open unix domain sockets (#2270) * Added code to automatically reformat VQL in notebook. (#2271) * [Snyk] Upgrade ace-builds from 1.12.4 to 1.12.5 (#2269) fix: upgrade ace-builds from 1.12.4 to 1.12.5 Snyk has created this PR to upgrade ace-builds from 1.12.4 to 1.12.5. See this package in npm: https://www.npmjs.com/package/ace-builds See this project in Snyk: https://app.snyk.io/org/scudette/project/76f4d127-566b-42ef-86f4-bdcbc92b90b4?utm_source=github&utm_medium=referral&page=upgrade-pr * Fixed the Windows.KapeFiles.Extract artifact (#2275) Refactor code to pass OSPath into VQL functions. Fixed bug in escaping the paths in the collector accessor. * Upgrade Velociraptor's yara plugin to support yara 4.2.3 (#2277) * Theme fixes and improvements (#2278) * Bugfix: Downloading CSV from table breaks with error. (#2280) Additional Version field was added to download handler but was not decoded by the schema parser properly. * Bugfix: uploads.json in the flow download refered to filestore paths (#2282) The _Components column of the uploads.json in the filestore refers to filestore paths (e.g. clients/C.1bfa6928675831f5-O123/collections/F.CE2PSBS6BQCSO...) This makes it incompatible with the offline collector which uses this column to refer to paths within the zip file itself. Previously this same file was just copied to the container but now it is being modified to support the different components list. This makes the upload.json format compatible with the offline collector so that VQL artifacts designed to work on the offline collector can also work on flow downloads. * Fix example for dummy proxy in documentation (#2281) Omitting the protocol results in an error at application start-up. * Theme fixes and improvements (#2283) * Ensure reserved user names can not be used (#2284) Previously reserved user names like VelociraptorServer were blocked from being added in VQL but it was still possible to use the command line to add these reserved user names. This PR adds additional contraints and a test to confirm these reserved user names are not added. * ACE editor font corrections (#2285) * Theme fixes and improvements * Fix ACE editor font names * Update font names _variables * Add shaded container around artifact description content (#2287) * Use 'HuntDescription' value for hunt() 'description' value (#2289) * User admin management screeb (#2212) Introduce a new user management UI that allows admins to manipulate user's org memberships, reset passwords, create new users and change user's roles and permissions. Special thanks to @jeffmahoney for working on this really large feature! * Theme fixes (#2291) * Theme fixes * Improved legibility of <code> in artifact viewer * Bugfix: Export notebook to zip broken (#2295) Also added a test and new VQL function create_notebook_export() * Added write_jsonl plugin. (#2296) * Bugfix: Collector timeout was set in ns (#2297) * Fix freebsd build (#2298) * Bump decode-uri-component from 0.2.0 to 0.2.2 in /gui/velociraptor (#2299) Bumps [decode-uri-component](https://github.com/SamVerschueren/decode-uri-component) from 0.2.0 to 0.2.2. - [Release notes](https://github.com/SamVerschueren/decode-uri-component/releases) - [Commits](SamVerschueren/decode-uri-component@v0.2.0...v0.2.2) --- updated-dependencies: - dependency-name: decode-uri-component dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Prepare for 0.6.7-2 release Incorporate bugfixes from 0.6.7-1 release. * Fix test * Fix test * Fix Windows.NTFS.MFT * Fixed test Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Snyk bot <snyk-bot@snyk.io> Co-authored-by: predictiple <predictiple@users.noreply.github.com> Co-authored-by: ae-govau <adam.eijdenberg@defence.gov.au> Co-authored-by: weslambert <wlambertts@gmail.com> Co-authored-by: Jeff Mahoney <jeffm@suse.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
This PR implements a user administration interface in the GUI. It requires some additional endpoints to list user roles, list organizations, get an individual user, create a user, update a user, and delete a user. The user interface is aware of organizations and does most of the right things -- but it follows the existing patterns where the system is capable of allowing different users to have different roles in each org, but nothing actually does it yet.