RosiKyu commented on PR #12973:
URL: https://github.com/apache/cloudstack/pull/12973#issuecomment-5280168335
@nvazquez, I have re-tested the PR on a 4.22 KVM environment, on a build
that includes the latest fix commit, alongside a 4.22 build without the PR.
Everything below is a measured before and after.
**The performance fix works as expected.**
- One `createAccount` by a Domain Admin, counted from the MySQL query log,
on both environments with `dynamic.apichecker.cache.period=0`:
| | without the PR | with the PR |
|---|---|---|
| database queries for role permissions | **10939** | **5** |
| total database queries | 11421 | 212 |
| time for the API call to return | **10633 ms** | ~1300 ms |
A single account creation took over 10 seconds on the build without the PR.
- Turning the cache on drops it further still. Three `createAccount` calls
in a row with `dynamic.apichecker.cache.period=300`:
```
run 1 (first call after restart) role permission queries = 2
run 2 role permission queries = 0
run 3 role permission queries = 0
```
The first call reads the permissions once, and later calls reuse them from
memory until the cache period runs out.
**Escalation is still correctly blocked**, and the new `Escalated APIs:
[...]` message is a real improvement because it names the exact API responsible:
```
Error: (HTTP 531, error code 4365) User of Account Account
[{"accountName":"restricted-da-user",...}] and domain Domain {...} cannot
create an account with access to more privileges than they have. Escalated
APIs: [createServiceOffering]
```
**The improved message for the missing role case works.** A default Domain
Admin creating a User account with the Root Admin role now gets:
```
Error: (HTTP 531, error code 4365) User of account: test-domadmin cannot
assign this role on the requested account: test-account
```
instead of the previous `has role null or unknown`, with the original detail
kept in the log. Much clearer.
**No regressions found.** All 17 account and user APIs affected by this
change were exercised as a Domain Admin against an account it owns: create,
update, enable, disable and delete account; create, update, enable, disable and
delete user; register and get user keys; move user; update resource limit and
count. All pass. Valid account creations are not wrongly refused, including a
restricted role creating an account that carries that same role.
## Two additional issues found
### 1. Revoking a permission does not take effect until the cache expires
Only happens when `dynamic.apichecker.cache.period` is greater than 0. The
default is 0, so default installations are not affected.
After a permission is revoked from a role, account creation carries on using
the remembered copy. A restricted Domain Admin can create an account that still
has the revoked permission, for as long as the cache period lasts. The window
is temporary but the account created in it is permanent, so this reopens #5781
for that period.
**Steps to reproduce**, with `cache.period=300` and the management server
restarted so nothing is remembered yet:
1. Grant `createServiceOffering` to the restricted user's role (a copy of
Domain Admin with that one rule denied)
2. Check in the database that the rule reads `ALLOW`
3. As that user, create an account with the default Domain Admin role. This
is allowed at this point, and it is the call that puts the permission into the
cache
4. Revoke `createServiceOffering` from the restricted user's role
5. Check in the database that the rule now reads `DENY`
6. Immediately create another account with the default Domain Admin role
Step 6 should be refused with `Escalated APIs: [createServiceOffering]`.
(**It succeeds instead.**)
Steps 3 to 6 have to be done within the cache period, counted from step 3,
because step 3 is what puts the permission into memory. If that time runs out
in between, the database is read again correctly and the problem does not show.
**Observed**, 49 seconds after step 3, with the database reading `DENY`:
```
account_name role created
test-acct-1 Domain Admin 09:37:27 <- step 3, allowed
test-acct-2 Domain Admin 09:38:16 <- step 6, should have been
refused
restricted user's role permission in database: DENY
test-acct-2 has createServiceOffering=ALLOW: yes
```
**The same sequence on a build without the PR refuses it immediately**, with
the same cache setting:
```
cache.period = 1800
[10:46:56] grant createServiceOffering database = ALLOW
[10:46:57] create #1 (puts permission in cache) -> CREATED
[10:47:01] revoke createServiceOffering database = DENY
[10:47:01] create #2 immediately -> REFUSED
[10:47:01] create #3 immediately -> REFUSED
```
Setting `cache.period` back to 0 on the PR build also makes it refuse
correctly every time.
This works in both directions. A newly *granted* permission also fails to
take effect until the cache period runs out. That is not a security problem,
but it will be reported as permissions not working.
**Suggestion.** Clear the remembered permissions for a role when they are
changed, or read them from the database for this particular check.
### 2. A long list of escalated APIs makes the API return HTTP 500 with an
empty response
This one does **not** need the cache. It happens on a default installation.
Every differing API name is put into the error message with no limit. Past a
certain length the API stops returning the error at all.
Tested by calling `createAccount` directly and checking the response, with
`dynamic.apichecker.cache.period=0`:
| escalated APIs | HTTP status | readable error |
|---|---|---|
| 326 | `531` | yes |
| 786 | `500` | no |
**The identical call on a build without the PR returns a proper error**:
| | without the PR | with the PR |
|---|---|---|
| HTTP status | **531** | **500** |
| readable error | yes | no |
**Steps to reproduce**, no special configuration needed:
1. Create a Domain Admin role with only a few rules allowed (`listApis`,
`listAccounts`, `listDomains`, `createAccount`, `createUser`, `listUsers`,
`listConfigurations`). An account on that role can see about 140 APIs.
2. Create an account with that role and generate API keys for it.
3. As that user, run `createAccount` with a much more privileged role. Using
the default Domain Admin role gives 326 escalated APIs and returns a proper
error. Using a role with a `*` ALLOW rule gives 786 and returns `500` with an
empty response.
cmk shows the failing case as:
```
(tpr_minimal) 🐱 > create account accounttype=2 username=tc-chk-c
password=Password123456 firstname=T lastname=T [email protected]
domainid=2a43e16e-f8b6-4a93-ba87-74ed1a5ecd9f
roleid=5c7e8978-d79c-43f8-b2a2-5201e7192d08
🙈 Error: failed to decode response
```
So the user gets no error text at all, and a `500` instead of the intended
`531`, which looks like the server broke rather than a permission being refused.
The escalation is still correctly refused and nothing is written to the
database, so this is not a security issue.
This also hides the improved message from `81df63ed5c0` whenever the list is
long, so that improvement is only visible when the list is short.
**Suggestion.** Limit the list in the message, for example the first 10
followed by `(+N more)`, keeping the full list in the log if it is needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
