Mailing-List: contact dev-help@cloudstack.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@cloudstack.apache.org
From: rhtyd <git@git.apache.org>
To: dev@cloudstack.apache.org
Reply-To: dev@cloudstack.apache.org
References: <git-pr-1489-cloudstack@git.apache.org>
In-Reply-To: <git-pr-1489-cloudstack@git.apache.org>
Subject: [GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based
 API C...
Content-Type: text/plain
Message-Id: <20160425162929.A95A8DFE60@git1-us-west.apache.org>
Date: Mon, 25 Apr 2016 16:29:29 +0000 (UTC)

Github user rhtyd commented on the pull request:

    https://github.com/apache/cloudstack/pull/1489#issuecomment-214430311
  
    > @rhtyd My comment regarding the test was more in the context of perf. test. In the DB for regular user I saw ~250 permissions got created. So this means iterating over all these entries twice (ALLOW and DENY) to find a match and then perform access check.
    
    The two checkPermissions calls would not cause significant overhead as they are done in memory and cost a maximum (worst case) of O(n) computation (on same machine). While making two DB calls (once to get all ALLOW rules and once to get all DENY rules) is more expensive as we do two network calls to get the data and still hit worst case O(n) computation.
    
    For a dynamic access checking system, this is a trade off and also a feature. In case of static checker we've now, rules are loaded at load-time only; any change requires restart and rules can be inconsistent across server(s).
    
    > There will be a perf. overhead due to this and user should have an option to decide whether to use static or dynamic.
    
    Users do have a choice, they can choose to not migrate to this feature. Typically, in production organizations would run multiple management servers; mgmt servers can be horizontally scaled to meet demanding usage. For example, they can can mgmt server on a separate machine (which they generally do) and tune their databases to accept up to 50k requests (or qps), optimize innodb settings (buffer pool size to 60% of memory etc.) and in db.properties increase num. of db connections from 250 to 1000.
    
    > Also if the user finds some issues/bugs later during their testing there should be a fallback option.
    
    There is a way to revert back to using static checker as I explained earlier, (1) switch off the global settings and (2) put back a commands.properties file in class-path usually at /etc/cloudstack/management; it's just that we don't want users to do it easily. Consider this an admin creates a read-only admin role and accounts with that (such users can only do list* calls), now if they go back to static-checker all such users now become default root admin (based on account type, translation) and can call all APIs defined in commands.properties -- this is a significant security risk. Therefore, I personally don't want to put users at risk and just discourage them using the static checker.
    
    > Regarding upgrade implications, I went through the docs/FS but some things are still confusing. If existing user can continue using commands.properties then what happens to the new APIs that gets added.
    
    In case you're not aware, with the current system each time a user upgrade they have to edit/add new rules to commands.properties by hand. Upgrading using packages does not update commands.properties file; it would create a .rpmnew/rpmsave file for example. In case of multiple mgmt server, you have to do this on all server(s) and restart all of them. While in case of dynamic roles based checker, you don't need to restart mgmt server at all (even during migration). I'm not sure why you say the static checker way is flexible, on the contrary I think it is not and a pain point of a lot of people.
    
    > If the argument is that the permission can be put in as an annotation in code for new APIs then that removes the flexibility of the earlier mechanism (there is no way to modify the default in code).
    
    This has existed for so long, but  not popular among API writers. Even before this feature, there have been few APIs using the annotation; the static checker also uses annotation as a fallback (i.e. not something I've introduced).
    
    There is a way to modify default behavior by adding authorized field in @APIParam. See the new APIs implementation for example. I've added a section in the FS on how new APIs should be written if they need to be enabled by default for a role type; alternatively the release notes should properly document new APIs and leave the choice of allowing/denying those APIs to (custom) roles.
    
    > We don't know how people are customising commands.properties and removing the flexibility may not be a good idea.
    
    On the contrary, we've giving flexibility to people. It might make sense to enable certain features for all role types or a subset of them. We have deny rules (with API and wildcards supported) in case they want to override the default. Consider this, you wrote an API and enabled for users; the system admin can explicitly add allow rules and add a \* deny rule that is to say deny all (if not allowed) and the dynamic roles system would not consider default rules in annotations at all.
    
    > The question is not about advantage of static checker, but more about choice and stability of the new mechanism.
    
    There is both choice and stability. We've tried our best to make this feature a drop in replacement that is strictly backward compatibility, with good integration tests and coverage on critical pieces. In fact, the integration tests run with Travis to ensure this becomes one of the critical smoke tests and aims to provide nearly 100% end-to-end coverage.
    
    If you've any code specific reservation or suggestions on improving it, I'll be happy to fix them.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---