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 [email protected] or file a JIRA ticket
with INFRA.
---
