API key pair restructure follow-ups

[bernardodemarco]

Description

A workflow introduced in the API key pair restructure tries to retrieve the accessing API key contained in HTTP requests by looking up for the apiKey string in a case-sensitive way. However, when verifying a request, the Management Server also accepts the API key to be specified in lowercase (apikey). The same behavior happens for the signature parameter.

Thus, if the API key is specified as apikey in HTTP requests, the key pair validation workflow does not identify the key used for authentication and it assumes that they are established via session. This behavior can leak key pairs with broader permission sets than the accessing pair actually has. These leaks can only happen for pairs belonging to the same user; one user from one account is not able to access keys from another user of another account.

Another incorrect behavior was found out, which allows for an accessing API key with a limited permission scope to register other pairs with all the permissions of the corresponding user's account. This is possible when no explicit permissions are defined and, under these circumstances, the registration workflow assumes the authentication was performed with an accessing pair without any explicit permissions as well.

This PR fixes all these reported issues.

Types of changes

• Breaking change (fix or feature that would cause existing functionality to change)
• New feature (non-breaking change which adds functionality)
• Bug fix (non-breaking change which fixes an issue)
• Enhancement (improves an existing feature and functionality)
• Cleanup (Code refactoring and cleanup, that may add test cases)
• Build/CI
• Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Bug Severity

• BLOCKER
• Critical
• Major
• Minor
• Trivial

Screenshots (if appropriate):

How Has This Been Tested?

• I created an API key pair with the following permissions. This pair was used for the execution of all described test cases, except when informed otherwise:

  API key pair permissions

    "permissions": [
      {
        "permission": "allow",
        "rule": "listUserKey*"
      },
      {
        "permission": "allow",
        "rule": "listApis"
      },
      {
        "permission": "allow",
        "rule": "registerUserKeys"
      },
      {
        "permission": "allow",
        "rule": "getUserKeys"
      }
    ]

Leak of key pairs belonging to the same user

• I executed the listUserKeys API with the key pair created in the previous step
  • I verified that its return did not include other API key pairs with larger permission sets
• I executed the getUserKeys API
  • I verified that its return did not include other API key pairs with larger permission sets

Registration of key pairs with a broader permission scope

• I executed the registerUserKeys API without specifying explicit rules
  • I verified that the new key pair permission set was equal to the set of the key pair used to perform the request
• I executed the registerUserKeys API specifying a rule set belonging to the set of the key pair used to perform the request
  • I verified that the new key pair was successfully registered
• I executed the registerUserKeys API specifying rules which the accessing key pair does not have access to
  • I verified the the command execution was not successful

[codecov]

Codecov Report

❌ Patch coverage is 22.95082% with 47 lines in your changes missing coverage. Please review.
✅ Project coverage is 19.65%. Comparing base (64178ea) to head (dee7dd3).

Files with missing lines	Patch %	Lines
...c/main/java/com/cloud/user/AccountManagerImpl.java	21.05%	28 Missing and 2 partials ⚠️
server/src/main/java/com/cloud/api/ApiServer.java	0.00%	7 Missing ⚠️
.../src/main/java/com/cloud/vm/UserVmManagerImpl.java	0.00%	3 Missing ⚠️
...oudstack/acl/DynamicRoleBasedAPIAccessChecker.java	60.00%	0 Missing and 2 partials ⚠️
...oudstack/acl/ProjectRoleBasedApiAccessChecker.java	0.00%	2 Missing ⚠️
.../cloudstack/discovery/ApiDiscoveryServiceImpl.java	0.00%	2 Missing ⚠️
...loudstack/acl/StaticRoleBasedAPIAccessChecker.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##               main   #13828   +/-   ##
=========================================
  Coverage     19.65%   19.65%           
- Complexity    19798    19803    +5     
=========================================
  Files          6368     6368           
  Lines        574943   574965   +22     
  Branches      70355    70361    +6     
=========================================
+ Hits         113015   113030   +15     
- Misses       449658   449660    +2     
- Partials      12270    12275    +5     

Flag	Coverage Δ
uitests	3.41% <ø> (ø)
unittests	20.93% <22.95%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[bernardodemarco]

@blueorangutan package

[blueorangutan]

@bernardodemarco a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18805

[DaanHoogland]

@blueorangutan test

[blueorangutan]

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

[blueorangutan]

[SF] Trillian test result (tid-16728)
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 58959 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr13828-t16728-kvm-ol8.zip
Smoke tests completed. 156 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

[bernardodemarco]

@blueorangutan package

[blueorangutan]

@bernardodemarco a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18865

[weizhouapache]

@blueorangutan test

[blueorangutan]

@weizhouapache a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

[blueorangutan]

[SF] Trillian test result (tid-16758)
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 60453 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr13828-t16758-kvm-ol8.zip
Smoke tests completed. 156 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

[weizhouapache]

thanks @bernardodemarco @winterhazel

@wido
if possible, can you ask Claude to assess this PR ?

[github-actions]

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

[bernardodemarco]

@blueorangutan package

[blueorangutan]

@bernardodemarco a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18898

[weizhouapache]

@blueorangutan test

[blueorangutan]

@weizhouapache a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

[blueorangutan]

[SF] Trillian test result (tid-16776)
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 57558 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr13828-t16776-kvm-ol8.zip
Smoke tests completed. 155 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_01_vpn_usage	Error	1.12	test_usage.py

[wido]

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.

[wido]

Tested with Claude Fable, this should go in. There is room for improvement later

[winterhazel]

Looks good. I tested basic API key pair operations (register, list, delete), and verified that the problematic scenarios do not occur anymore (listUserKeys leaking key pairs with more permissions due to the case-sensitivity issue, registerUserKeys registering keys with broader permission sets when no explicit permissions are defined, key pair having access to all the account's permissions when all the explicitly configured permissions are filtered by the role).

@bernardodemarco these are not caused by your changes, but a few issues that I noticed which we should improve in a future release:

• count of listUserKeys returns the total number of API key pairs, without the filtering. Below, I called listUserKeys for a user with 13 keys, using a key pair that should have access only to 2 key pairs. count was 13, but only 2 keys were returned. The keys themselves are not leaked, so we can release 4.23 with this.

(a) 🐱 > list userkeys
{
  "count": 13,
  "userapikey": [
    {
      "accountid": "a7bccbbb-30a8-40f1-b949-cac24c5b94fd",
      "accountname": "a",
      "accounttype": "DOMAIN_ADMIN",
      "apikey": "n-RopdRGrVR0yl9QH3ZS-8YjNSQ9WQGzIkye0GQCA1ErSZwQqam_bbhfXXinJiZNyv2mEIObss-4JUMx9X4mJg",
      "created": "2026-08-20T08:51:52-0300",
      "domain": "ROOT",
      "domainid": "d1dca945-17c4-11f1-9235-32e0826870ba",
      "domainpath": "ROOT",
      "id": "470af8ca-3015-4903-b0ed-dfa02da90669",
      "name": "teste",
      "roleid": "7e0f0ea1-f1d1-42a2-935c-4277c4f358a8",
      "rolename": "a",
      "roletype": "DomainAdmin",
      "secretkey": "AD1JkcFdW9bYQWyrcfM_K08Sp100Lv-IMmjeMm1COKl14WBYjnG6QQxhL0By8JOmCGdWrdW74zACNtP8P8bTRQ",
      "state": "ENABLED",
      "userid": "df4ff7f0-fbd6-4ff7-a5fb-ff6ba97ead0d",
      "username": "a"
    },
    {
      "accountid": "a7bccbbb-30a8-40f1-b949-cac24c5b94fd",
      "accountname": "a",
      "accounttype": "DOMAIN_ADMIN",
      "apikey": "m0w3lheU8GkWE0Li89p4zmfVTo_5h9EtpXTuZmBbaB1y4CUU8N9DKuLorR4zp2jKE-Te5msqnPPZshcmBTWT6g",
      "created": "2026-08-20T08:52:27-0300",
      "domain": "ROOT",
      "domainid": "d1dca945-17c4-11f1-9235-32e0826870ba",
      "domainpath": "ROOT",
      "id": "8f718160-0857-424e-bbd3-cd291ef43781",
      "name": "5 - API key pair",
      "roleid": "7e0f0ea1-f1d1-42a2-935c-4277c4f358a8",
      "rolename": "a",
      "roletype": "DomainAdmin",
      "secretkey": "aeeF4ytzZROiEhKqPKFoj2TvDlUM_ytMbNDzJbtDlCefvj87AVTfsnNRidqG-SUahi6g27Ux3tBhFfc2jXpfqw",
      "state": "ENABLED",
      "userid": "df4ff7f0-fbd6-4ff7-a5fb-ff6ba97ead0d",
      "username": "a"
    }
  ]
}

• listUserKeys is very slow when a user has multiple keys. Below, listing the API key pairs for a user with 13 keys took 4 seconds.

2026-08-20 08:42:50,955 DEBUG [c.c.a.ApiServlet] (qtp1390913202-19:[ctx-6e92a473]) (logid:190e3549) ===START===  192.168.32.1 -- POST  
2026-08-20 08:42:50,959 DEBUG [c.c.a.ApiServer] (qtp1390913202-19:[ctx-6e92a473, ctx-ceb82968]) (logid:190e3549) CIDRs from which account 'Account [{"accountName":"a","id":5,"uuid":"a7bccbbb-30a8-40f1-b949-cac24c5b94fd"}]' is allowed to perform API calls: 0.0.0.0/0,::/0
2026-08-20 08:42:50,961 DEBUG [o.a.c.a.StaticRoleBasedAPIAccessChecker] (qtp1390913202-19:[ctx-6e92a473, ctx-ceb82968]) (logid:190e3549) RoleService is enabled. We will use it instead of StaticRoleBasedAPIAccessChecker.
2026-08-20 08:42:50,961 DEBUG [o.a.c.r.ApiRateLimitServiceImpl] (qtp1390913202-19:[ctx-6e92a473, ctx-ceb82968]) (logid:190e3549) API rate limiting is disabled. We will not use ApiRateLimitService.
2026-08-20 08:42:51,021 DEBUG [c.c.a.ApiServer] (qtp1390913202-19:[ctx-6e92a473, ctx-ceb82968, ctx-0e20f7ef]) (logid:190e3549) CIDRs from which account 'Account [{"accountName":"a","id":5,"uuid":"a7bccbbb-30a8-40f1-b949-cac24c5b94fd"}]' is allowed to perform API calls: 0.0.0.0/0,::/0
2026-08-20 08:42:51,023 DEBUG [o.a.c.a.StaticRoleBasedAPIAccessChecker] (qtp1390913202-19:[ctx-6e92a473, ctx-ceb82968, ctx-0e20f7ef]) (logid:190e3549) RoleService is enabled. We will use it instead of StaticRoleBasedAPIAccessChecker.
2026-08-20 08:42:51,023 DEBUG [o.a.c.r.ApiRateLimitServiceImpl] (qtp1390913202-19:[ctx-6e92a473, ctx-ceb82968, ctx-0e20f7ef]) (logid:190e3549) API rate limiting is disabled. We will not use ApiRateLimitService.
2026-08-20 08:42:51,023 INFO  [c.c.a.ApiServer] (qtp1390913202-19:[ctx-6e92a473, ctx-ceb82968, ctx-0e20f7ef]) (logid:190e3549) API accessed through API Key Pair. API Key: [2K6LqxlPxOiuEMQyCKYJKKkDO2Cf3UFk1poHUDydJrwDIHlvtfSFymwv2fuNpkST-pTALjpYutWTYpeISnCOHA].
2026-08-20 08:42:51,025 INFO  [c.c.u.AccountManagerImpl] (qtp1390913202-19:[ctx-6e92a473, ctx-ceb82968, ctx-0e20f7ef]) (logid:190e3549) Request's API key is [2K6LqxlPxOiuEMQyCKYJKKkDO2Cf3UFk1poHUDydJrwDIHlvtfSFymwv2fuNpkST-pTALjpYutWTYpeISnCOHA].
2026-08-20 08:42:51,381 INFO  [c.c.u.AccountManagerImpl] (qtp1390913202-19:[ctx-6e92a473, ctx-ceb82968, ctx-0e20f7ef]) (logid:190e3549) Request's API key is [2K6LqxlPxOiuEMQyCKYJKKkDO2Cf3UFk1poHUDydJrwDIHlvtfSFymwv2fuNpkST-pTALjpYutWTYpeISnCOHA].
2026-08-20 08:42:51,738 INFO  [c.c.u.AccountManagerImpl] (qtp1390913202-19:[ctx-6e92a473, ctx-ceb82968, ctx-0e20f7ef]) (logid:190e3549) Request's API key is [2K6LqxlPxOiuEMQyCKYJKKkDO2Cf3UFk1poHUDydJrwDIHlvtfSFymwv2fuNpkST-pTALjpYutWTYpeISnCOHA].
2026-08-20 08:42:52,107 INFO  [c.c.u.AccountManagerImpl] (qtp1390913202-19:[ctx-6e92a473, ctx-ceb82968, ctx-0e20f7ef]) (logid:190e3549) Request's API key is [2K6LqxlPxOiuEMQyCKYJKKkDO2Cf3UFk1poHUDydJrwDIHlvtfSFymwv2fuNpkST-pTALjpYutWTYpeISnCOHA].
2026-08-20 08:42:52,469 INFO  [c.c.u.AccountManagerImpl] (qtp1390913202-19:[ctx-6e92a473, ctx-ceb82968, ctx-0e20f7ef]) (logid:190e3549) Request's API key is [2K6LqxlPxOiuEMQyCKYJKKkDO2Cf3UFk1poHUDydJrwDIHlvtfSFymwv2fuNpkST-pTALjpYutWTYpeISnCOHA].
2026-08-20 08:42:52,827 INFO  [c.c.u.AccountManagerImpl] (qtp1390913202-19:[ctx-6e92a473, ctx-ceb82968, ctx-0e20f7ef]) (logid:190e3549) Request's API key is [2K6LqxlPxOiuEMQyCKYJKKkDO2Cf3UFk1poHUDydJrwDIHlvtfSFymwv2fuNpkST-pTALjpYutWTYpeISnCOHA].
2026-08-20 08:42:53,194 INFO  [c.c.u.AccountManagerImpl] (qtp1390913202-19:[ctx-6e92a473, ctx-ceb82968, ctx-0e20f7ef]) (logid:190e3549) Request's API key is [2K6LqxlPxOiuEMQyCKYJKKkDO2Cf3UFk1poHUDydJrwDIHlvtfSFymwv2fuNpkST-pTALjpYutWTYpeISnCOHA].
2026-08-20 08:42:53,552 INFO  [c.c.u.AccountManagerImpl] (qtp1390913202-19:[ctx-6e92a473, ctx-ceb82968, ctx-0e20f7ef]) (logid:190e3549) Request's API key is [2K6LqxlPxOiuEMQyCKYJKKkDO2Cf3UFk1poHUDydJrwDIHlvtfSFymwv2fuNpkST-pTALjpYutWTYpeISnCOHA].
2026-08-20 08:42:53,910 INFO  [c.c.u.AccountManagerImpl] (qtp1390913202-19:[ctx-6e92a473, ctx-ceb82968, ctx-0e20f7ef]) (logid:190e3549) Request's API key is [2K6LqxlPxOiuEMQyCKYJKKkDO2Cf3UFk1poHUDydJrwDIHlvtfSFymwv2fuNpkST-pTALjpYutWTYpeISnCOHA].
2026-08-20 08:42:54,267 INFO  [c.c.u.AccountManagerImpl] (qtp1390913202-19:[ctx-6e92a473, ctx-ceb82968, ctx-0e20f7ef]) (logid:190e3549) Request's API key is [2K6LqxlPxOiuEMQyCKYJKKkDO2Cf3UFk1poHUDydJrwDIHlvtfSFymwv2fuNpkST-pTALjpYutWTYpeISnCOHA].
2026-08-20 08:42:54,621 INFO  [c.c.u.AccountManagerImpl] (qtp1390913202-19:[ctx-6e92a473, ctx-ceb82968, ctx-0e20f7ef]) (logid:190e3549) Request's API key is [2K6LqxlPxOiuEMQyCKYJKKkDO2Cf3UFk1poHUDydJrwDIHlvtfSFymwv2fuNpkST-pTALjpYutWTYpeISnCOHA].
2026-08-20 08:42:54,980 DEBUG [c.c.a.ApiServlet] (qtp1390913202-19:[ctx-6e92a473, ctx-ceb82968, ctx-0e20f7ef]) (logid:190e3549) ===END===  192.168.32.1 -- POST  

[weizhouapache]

I think this should be moved from line 1156 to line 1160, right ?

            CallContext.register(user, account);

cc @bernardodemarco @winterhazel

[weizhouapache]

btw, it looks like this additional check will increase the latency of request.
have you tested it ?

cc @bernardodemarco @winterhazel

[bernardodemarco]

Hello, @weizhouapache

I think this should be moved from line 1156 to line 1160, right ?

I'm not sure this would cause any real effect. Currently, if a key pair does not have access to a certain command, CallContext is registered, but, at the next step, verifyRequest returns false, which causes the API to be aborted with an error message.

it looks like this additional check will increase the latency of request. have you tested it ?

I did not test the performance of the workflow; my focus was targeted more to accuracy. But, IMHO, the workflows involving API key pairs authentication/authorization have vast room for improvement regarding performance... We can address them incrementally in the future.