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 11.76471% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 19.64%. Comparing base (5a67f19) to head (c0a3341).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...c/main/java/com/cloud/user/AccountManagerImpl.java	12.90%	25 Missing and 2 partials ⚠️
.../src/main/java/com/cloud/vm/UserVmManagerImpl.java	0.00%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #13828      +/-   ##
============================================
- Coverage     19.64%   19.64%   -0.01%     
+ Complexity    19790    19785       -5     
============================================
  Files          6368     6368              
  Lines        574889   574904      +15     
  Branches      70353    70358       +5     
============================================
- Hits         112962   112937      -25     
- Misses       449656   449694      +38     
- Partials      12271    12273       +2     

Flag	Coverage Δ
uitests	3.41% <ø> (ø)
unittests	20.91% <11.76%> (-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