cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From rhtyd <...@git.apache.org>
Subject [GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Date Mon, 25 Apr 2016 16:29:29 GMT
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.
---

Mime
View raw message