wido commented on PR #13828:
URL: https://github.com/apache/cloudstack/pull/13828#issuecomment-5348022549
> thanks @bernardodemarco @winterhazel
>
> @wido if possible, can you ask Claude to assess this PR ?
I had Claude Fable do a test and write a short comment:
Did a security review of this PR, focused on the API key auth path. The
three fixes look correct and this should go in — the case-sensitivity bug is
worse than the description suggests: since `ApiConstants.API_KEY` is `"apikey"`
and the old code did `getFullUrlParams().get("apiKey")`, `getAccessingApiKey()`
returned `null` for *every* real request, so all key pair scoping was silently
disabled. A restricted key could `listUserKeys` and read the `secretkey` of
unrestricted pairs.
A few things I'd like to see addressed:
**1. Scoping is still derived from raw request params (main concern).**
`ApiServer.verifyRequest` picks the api key by sorted-name iteration, last
match wins (ApiServer.java:1033-1053). `getAccessingApiKey` uses `findFirst()`
over a HashMap (AccountManagerImpl.java:3401). Param names are case-sensitive,
so `?apiKey=X&apikey=Y` gives two entries: auth binds Y, scoping can resolve X.
If X is a broader pair of the same user, `getKeys` returns its secret key. If X
is unknown, `checkApiAccess(caller, command, apiKey)` finds no pair and falls
back to the account role — the same fallback the checker fix removes (reachable
via `destroyVirtualMachine?expunge=true`).
`verifyRequest` already has the authenticated `ApiKeyPair` at line 1111 but
throws it away. Could we put it on `CallContext` and read it there instead of
re-parsing? Interim: reject requests with more than one case-variant of
`apikey`/`signature`.
**2. Fail-open.** `catch (NullPointerException) -> return null` means
"session auth", i.e. the widest permission set. `getFullUrlParams()` is null
under the async dispatcher, and `Gson.fromJson` can throw `JsonSyntaxException`
which isn't caught. Should fail closed.
**3. API key logged in cleartext.** `logger.info("Request's API key is
[{}]", apiKey)` (:3405). `ApiServer` keeps `"apikey"` in `sensitiveFields` to
mask exactly this. Please drop it or log the key pair UUID.
**4. Minor:**
- `UserVmManagerImpl:3683` reimplements the lookup and skips the signature
check — just call `getAccessingApiKey(cmd)`.
- `getAllKeypairPermissions` (:3676) and `getKeys` (:3267) deref
`findByApiKey()` without a null check → 500 on a bogus key, now more reachable.
- `getAllExplicitKeyPairPermissions()` is an uncached DB hit on every API
call, while role permissions are cached. `ApiServer` called
`findAllPermissionsByKeyPairId` one line earlier — could that return the flag
instead?
- `DynamicRoleBasedAPIAccessCheckerTest` only passes `null` for the new
param, so the `keyPairHasExplicitPermissions` branch — the actual fix — is
untested.
Also worth noting the static and project role checkers now take an
`ApiKeyPair` and ignore it, so key pair scoping isn't enforced there at all.
Pre-existing, but maybe worth a follow-up issue.
--
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]
