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