CLOUDSTACK-8562: Dynamic Role-Based API Checker for CloudStack #1489
Conversation
test_staticroles.py=== TestName: test_static_role_account_acls | Status : SUCCESS === scripts/util/migrate-dynamicroles.pyI'll add docs for this feature in a separate PR to admin-docs, this is the usage information: Usage: migrate-dynamicroles.py [options] Options: $ sudo ./scripts/util/migrate-dynamicroles.py Running this migration tool will remove any default-role rules in cloud.role_permissions. Do you want to continue? [y/N]y test_dynamicroles.py=== TestName: test_default_role_deletion | Status : SUCCESS === === TestName: test_role_account_acls | Status : SUCCESS === === TestName: test_role_account_acls_multiple_mgmt_servers | Status : SUCCESS === === TestName: test_role_inuse_deletion | Status : SUCCESS === === TestName: test_role_lifecycle_create | Status : SUCCESS === === TestName: test_role_lifecycle_delete | Status : SUCCESS === === TestName: test_role_lifecycle_list | Status : SUCCESS === === TestName: test_role_lifecycle_update | Status : SUCCESS === === TestName: test_role_lifecycle_update_role_inuse | Status : SUCCESS === === TestName: test_rolepermission_lifecycle_create | Status : SUCCESS === === TestName: test_rolepermission_lifecycle_delete | Status : SUCCESS === === TestName: test_rolepermission_lifecycle_list | Status : SUCCESS === === TestName: test_rolepermission_lifecycle_update | Status : SUCCESS === === TestName: test_rolepermission_lifecycle_update_invalid_rule | Status : SUCCESS === |
rohityadavcloud
force-pushed
the
dynamicroles-master
branch
from
5e5ee17 to
239b86b
Compare
|
This is a feature that we definitely need. Can I please get some code review on this PR? |
| @@ -143,7 +143,7 @@ def __sign(self, payload): | |||
| ["=".join( | |||
| [str.lower(r[0]), | |||
| str.lower( | |||
| urllib.quote_plus(str(r[1])) | |||
| urllib.quote_plus(str(r[1]), safe="*") | |||
There was a problem hiding this comment.
This is ported from cloudmonkey -- https://github.com/apache/cloudstack-cloudmonkey/pull/11/files
When tests send * in the cmd arg, test failures happened due to how url encoding differs across ACS specific Java code and Python
|
Some screenshots * Roles Tab: Add a new role: Adding rules to a role with API auto-completion ** (API and wildcards allowed): * feature available for root admin only |
|
@DaanHoogland I was pushing some changes and adding some info/screenshots above. Since this aims to deprecate use of commands.properties (affecting marvin and apidocs) I've a separate commit for that in this PR. All changes pushed from my end now. |
|
@bhaisaab, ok will continue tomorrow. |
rohityadavcloud
force-pushed
the
dynamicroles-master
branch
from
b11604d to
6246c05
Compare
| boolean updateRolePermission(final RolePermission rolePermission, final Rule rule, final RolePermission.Permission permission, final String description); | ||
| boolean deleteRolePermission(final RolePermission rolePermission); | ||
|
|
||
| List<Role> findAllRolesBy(final Long id, final String name, final RoleType roleType); |
There was a problem hiding this comment.
findAllRolesBy all three parameters? or either one of them? I will probably find out reading on but a comment/javadoc in this case would be nice (you know I'm not a fan of comments, right ;)
There was a problem hiding this comment.
This method tries to solve data processing for listRoles API, it can list by id (i.e. if exists, single item returned) , or returns list of role matching a name, or if nothing else passed checks if a role type was passed. @DaanHoogland do you want me to split them up into separate methods?
There was a problem hiding this comment.
@bhaisaab, you could split them or add a javadoc so users get a clue. If the caller is in control splitting makes sense. They'd know what they are passing anyway.
There was a problem hiding this comment.
I would have to agree with @DaanHoogland. Splitting into separate methods would be clearer and probably simplify the business logic per method as well (so it would be simpler to unit test).
rohityadavcloud
force-pushed
the
dynamicroles-master
branch
from
6246c05 to
1ba3477
Compare
| response.setResponses(rolePermissionResponses); | ||
| response.setResponseName(getCommandName()); | ||
| setResponseObject(response); | ||
| } |
There was a problem hiding this comment.
same as create commands: factor out?
|
One blocker found so far, some license headers missing. |
|
@DaanHoogland thanks, I'll fix them |
CI bubble runinitially test_02_redundant_VPC_default_routes failed. On manual verification it succeeded. |
|
thanks @DaanHoogland |
| roleType = RoleType.DomainAdmin; | ||
| break; | ||
| case Account.ACCOUNT_TYPE_RESOURCE_DOMAIN_ADMIN: | ||
| roleType = RoleType.ResourceAdmin; |
There was a problem hiding this comment.
What is this role type? Is there any documentation on it?
There was a problem hiding this comment.
All four default cloudstack roles -- Admin, DomainAdmin, ResourceAdmin and User. It's just an enum class, that maps these role types to account types. This method previously existed in account mgr impl, I moved it here.
They've always existed. I could not find a previous FS on this, this is the oldest document on wiki referring to RoleTypes https://cwiki.apache.org/confluence/display/CLOUDSTACK/Annotations+use+in+the+API
There was a problem hiding this comment.
I meant the ResourceAdmin specifically. What is this role? Is there any documentation on it?
There was a problem hiding this comment.
@pdube I think this was used to support cpbm-cloudstack integration, I don't know any documentation; please google? For backward compatibility reasons, all four roles types supported by static-role-based-checked (as in commands.properties) need to be supported.
There was a problem hiding this comment.
Thanks. I had seen it in the code and looked for more information about it, but didn't find anything conclusive
CLOUDSTACK-8562: Dynamic Role-Based API Checker for CloudStack### CLOUDSTACK-8562: DB-Backed Dynamic Role Based API Access Checker This feature allows root administrators to define new roles and associate API permissions to them. A limited form of role-based access control for the CloudStack management server API is provided through a properties file, commands.properties, embedded in the WAR distribution. Therefore, customizing API permissions requires unpacking the distribution and modifying this file consistently on all servers. The old system also does not permit the specification of additional roles. FS: https://cwiki.apache.org/confluence/display/CLOUDSTACK/Dynamic+Role+Based+API+Access+Checker+for+CloudStack DB-Backed Dynamic Role Based API Access Checker for CloudStack brings following changes, features and use-cases: - Moves the API access definitions from commands.properties to the mgmt server DB - Allows defining custom roles (such as a read-only ROOT admin) beyond the current set of four (4) roles - All roles will resolve to one of the four known roles types (Admin, Resource Admin, Domain Admin and User) which maintains this association by requiring all new defined roles to specify a role type. - Allows changes to roles and API permissions per role at runtime including additions or removal of roles and/or modifications of permissions, without the need of restarting management server(s) Upgrade/installation notes: - The feature will be enabled by default for new installations, existing deployments will continue to use the older static role based api access checker with an option to enable this feature - During fresh installation or upgrade, the upgrade paths will add four default roles based on the four default role types - For ease of migration, at the time of upgrade commands.properties will be used to add existing set of permissions to the default roles. cloud.account will have a new role_id column which will be populated based on default roles as well Dynamic-roles migration tool: scripts/util/migrate-dynamicroles.py - Allows admins to migrate to the dynamic role based checker at a future date - Performs a harder one-way migrate and update - Migrates rules from existing commands.properties file into db and deprecates it - Enables an internal hidden switch to enable dynamic role based checker feature * pr/1489: maven: Fix jstl version usage CLOUDSTACK-8562: Deprecate commands.properties CLOUDSTACK-8562: DB-Backed Dynamic Role Based API Access Checker CLOUDSTACK-9361: Centrally handle API validations Signed-off-by: Will Stevens <[email protected]>
|
Thanks you @swill finally 😄 |
|
@koushik-das @swill @rhtyd Some APIs are giving permission exceptions. They can be seen by just logging into UI. I have added the commands.properties to overcome these exceptions in my setup. |
|
@anshul1886 can you share the details, which APIs. Also, are you facing this on master or on your personal/feature branch? |
|
@rhtyd There are many APIs. I have not prepared any list. But creating setup from scratch should reproduce it. Some operations in which it can be observed
|
|
@anshul1886 I'm unable to reproduce the shared issues nor they were caught by CI/Jenkins/Travis runs. The role specific rules in role_permissions table have been copied from the last commands.properties.in file. If you've added new APIs they need to use authorized annotations. |
|
@rhtyd Is commands.properties exist in your setup or not? If it exists then things are working fine. |
|
@anshul1886 with dynamic-roles feature, we no longer want to use or promote use of commands.properties file which is why this has been removed in master. We no longer want to add changes/new APIs to this file as well though existing installations will continue to use the static-checker until they decide to upgrade. You need to use the annotations as described on the wiki (this has been shared recently on dev ML as well): I think you've not rebuilt your branch after a rebase, you need to perform a clean rebuild. |
|
@rhtyd I have rebuilt my branch which was rebased yesterday. I am creating new setup. There is no new API introduced in my branch. I am talking about existing APIs which are failing. After putting commands.properties I am not facing issues. But as you pointed that is not what is intended with this PR. |
|
@anshul1886 okay, let's debug;
|
|
If we have to revert this, it will be a mess. Lets try to get to the bottom of this ASAP as the freeze is basically here... |
|
@swill Travis and your CI are passing with this feature during which a data center is deployed and all sorts of APIs are called so it's likely an env issue specific for @anshul1886. We've put a lot of testing effort on this including various upgrade scenarios, and have seen huge amount of effort on code review so it is not needed to revert the feature; though if there is a valid bug it ought to be fixed and that's what freeze is about -- fixing bugs. Here what I'm speculating where it is failing: Lastly, even after request I'm yet to see some valid logs, exception, screenshots etc from @anshul1886 that would help me debug and fix the issue, if any. |
|
I agree. But I have not tested the UI at all. Has your team been testing through the UI as well as through the API? |
|
@swill yes sir. I'll personally make sure to perform another test round tomorrow both with UI, marvin and APIs/cloudmonkey. |
|
@rhtyd thank you sir. I am trying to get everything ready to freeze. Damn this is a lot of work. :( |
|
@rhtyd Its a clean setup. It is not even complete because of one other bug introduced by PR #816. Summary of the steps:
For commit history on my setup see PR #351. And that commits code is not even coming into picture. To go ahead I have disabled dynamic.apichecker.enabled and copied the commands.properties file. After that I was blocked by the bug introduced by #816. So I have not completed my setup. |
|
@anshul1886 I'm yet to see chrome dev-tool screenshots, actual errors reported in the UI and see answers for questions I asked; again -- the role_id in accounts table, your environment info (win/linux/osx etc). Now, I cannot guarantee that this will work in your feature branch as I don't know what changes your branch has; I've only tested master with a fresh environment on a linux machine and it is working for me both using maven and using rpms http://packages.shapeblue.com/cloudstack/custom/master-rbac:
Created a user with user role:
Was able to log in:
|
|
@rhtyd Same results on master. I have not done anything in setup. My dev box is ubuntu 12.04 |
|
@rhtyd Default value for dynamic.apichecker.enabled is true. role_id for default account is 1. In logs only exception I am getting is |
|
@anshul1886 is this environment installed using rpms/deb or it's a developer machine with Ubuntu 12.04? Would you prefer a GTM to get to the bottom of the issue, instead of back and forth on PR/ML? |
|
dev machine which I am using for last 2-3 years |
|
@anshul1886 if you need help join this GTM now: https://app.gotomeeting.com/?meetingId=981540549 (or use the meeting id for a desktop client). I've personally tested master and I think the issues you're presenting are your specific env related. I'll wait for you for about 15-20 mins. |
|
@rhtyd @anshul1886 This week I rebased the ospf changes with current master and did not have any issues. OSPF adds 2 new APIs and they are using the new scheme of things, looked good to Marvin and UI. |
|
@swill I had a GTM with Anshul to figure out the issue, his environment is not clean and I've asked him to setup a new environment (effectively a new dev box). I've shared notes with him that if |
|
@rhtyd It looks fine now. It was present in some tomcat folder. |
|
Thanks guys for getting that sorted out. Is the problems that @anshul1886 ran into something that others could potentially run into? Would it be useful to document the problem and the solution so others can benefit from it if they run into it? |
|
@swill the problem was environment dependent where an unclean environment was used to build/run management server, it is recommended to git clean -fdx one's local repository (or at least run a mvn clean -P developer,systemvm -Dnoredist -Dsimulator) to get rid of old files. The case has been documented on the FS that for |
|
@rhtyd thanks, just wanted to make sure we extracted anything that was useful from @anshul1886's experience. Thanks for working this out guys... |
CLOUDSTACK-8562: DB-Backed Dynamic Role Based API Access Checker
This feature allows root administrators to define new roles and associate API
permissions to them.
A limited form of role-based access control for the CloudStack management server
API is provided through a properties file, commands.properties, embedded in the
WAR distribution. Therefore, customizing API permissions requires unpacking the
distribution and modifying this file consistently on all servers. The old system
also does not permit the specification of additional roles.
FS:
https://cwiki.apache.org/confluence/display/CLOUDSTACK/Dynamic+Role+Based+API+Access+Checker+for+CloudStack
DB-Backed Dynamic Role Based API Access Checker for CloudStack brings following
changes, features and use-cases:
current set of four (4) roles
Admin, Domain Admin and User) which maintains this association by requiring
all new defined roles to specify a role type.
removal of roles and/or modifications of permissions, without the need
of restarting management server(s)
Upgrade/installation notes:
deployments will continue to use the older static role based api access checker
with an option to enable this feature
roles based on the four default role types
to add existing set of permissions to the default roles. cloud.account
will have a new role_id column which will be populated based on default roles
as well
Dynamic-roles migration tool: scripts/util/migrate-dynamicroles.py