You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "bernardodemarco (via GitHub)" <gi...@apache.org> on 2026/08/08 13:47:41 UTC

[PR] API key pair restructure follow-ups [cloudstack]

bernardodemarco opened a new pull request, #13828:
URL: https://github.com/apache/cloudstack/pull/13828

   ### 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)
   - [X] 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
   - [X] 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:
       <details>
   
       <summary>API key pair permissions</summary>
   
       ```json
         "permissions": [
           {
             "permission": "allow",
             "rule": "listUserKey*"
           },
           {
             "permission": "allow",
             "rule": "listApis"
           },
           {
             "permission": "allow",
             "rule": "registerUserKeys"
           },
           {
             "permission": "allow",
             "rule": "getUserKeys"
           }
         ]
       ```
   
       </details>
   
   ### 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
    
   


-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] API key pair restructure follow-ups [cloudstack]

Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #13828:
URL: https://github.com/apache/cloudstack/pull/13828#issuecomment-5313011572

   @blueorangutan test


-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] API key pair restructure follow-ups [cloudstack]

Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #13828:
URL: https://github.com/apache/cloudstack/pull/13828#issuecomment-5327669171

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


-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] API key pair restructure follow-ups [cloudstack]

Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13828:
URL: https://github.com/apache/cloudstack/pull/13828#issuecomment-5241321275

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


-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] API key pair restructure follow-ups [cloudstack]

Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13828:
URL: https://github.com/apache/cloudstack/pull/13828#issuecomment-5313017889

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


-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] API key pair restructure follow-ups [cloudstack]

Posted by "winterhazel (via GitHub)" <gi...@apache.org>.
winterhazel commented on code in PR #13828:
URL: https://github.com/apache/cloudstack/pull/13828#discussion_r3775251244


##########
server/src/main/java/com/cloud/user/AccountManagerImpl.java:
##########
@@ -3274,17 +3277,17 @@ public ListResponse<ApiKeyPairResponse> listKeys(ListUserKeysCmd cmd) {
         List<ApiKeyPairResponse> responses = new ArrayList<>();
 
         if (cmd.getKeyId() != null || cmd.getApiKeyFilter() != null) {
-            fetchOnlyOneKeyPair(responses, cmd);
+            populateSingleKeyPairResponse(responses, cmd);
             finalResponse.setResponses(responses);
             return finalResponse;
         }
 
-        Integer total = fetchMultipleKeyPairs(responses, cmd);
-        finalResponse.setResponses(responses, total);
+        populateMultipleKeyPairsResponse(responses, cmd);
+        finalResponse.setResponses(responses);

Review Comment:
   @bernardodemarco this breaks pagination of `listUserKeys`, even in the UI. I have 12 keys below, but it only shows 10.
   
   <img width="3189" height="1813" alt="Image" src="https://github.com/user-attachments/assets/5b56e6ca-d562-4b4c-b468-9cf8820a999e" /> 



-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] API key pair restructure follow-ups [cloudstack]

Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13828:
URL: https://github.com/apache/cloudstack/pull/13828#issuecomment-5329006355

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


-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] API key pair restructure follow-ups [cloudstack]

Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13828:
URL: https://github.com/apache/cloudstack/pull/13828#issuecomment-5250216272

   <b>[SF] Trillian test result (tid-16728)</b>
   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
   --- | --- | --- | ---
   


-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] API key pair restructure follow-ups [cloudstack]

Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #13828:
URL: https://github.com/apache/cloudstack/pull/13828#issuecomment-5241297080

   @blueorangutan test


-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] API key pair restructure follow-ups [cloudstack]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13828:
URL: https://github.com/apache/cloudstack/pull/13828#issuecomment-5327773067

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


-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] API key pair restructure follow-ups [cloudstack]

Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on code in PR #13828:
URL: https://github.com/apache/cloudstack/pull/13828#discussion_r3830938269


##########
server/src/main/java/com/cloud/api/ApiServer.java:
##########
@@ -1156,7 +1156,7 @@ public boolean verifyRequest(final Map<String, Object[]> requestParameters, fina
             CallContext.register(user, account);
 
             List<ApiKeyPairPermission> keyPairPermissions = keyPairManager.findAllPermissionsByKeyPairId(keyPair.getId(), account.getRoleId());
-            if (commandAvailable(remoteAddress, commandName, user, keyPairPermissions.toArray(new ApiKeyPairPermission[0]))) {
+            if (commandAvailable(remoteAddress, commandName, user, keyPair, keyPairPermissions.toArray(new ApiKeyPairPermission[0]))) {
                 logger.info("API accessed through API Key Pair. API Key: [{}].", keyPair.getApiKey());

Review Comment:
   btw, it looks like this additional check will increase the latency of request.
   have you tested it ? 
   
   cc @bernardodemarco @winterhazel 



-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] API key pair restructure follow-ups [cloudstack]

Posted by "bernardodemarco (via GitHub)" <gi...@apache.org>.
bernardodemarco commented on PR #13828:
URL: https://github.com/apache/cloudstack/pull/13828#issuecomment-5226661427

   @blueorangutan package


-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] API key pair restructure follow-ups [cloudstack]

Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #13828:
URL: https://github.com/apache/cloudstack/pull/13828#issuecomment-5226413797

   ## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/13828?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   :white_check_mark: All modified and coverable lines are covered by tests.
   :white_check_mark: Project coverage is 3.41%. Comparing base ([`5a67f19`](https://app.codecov.io/gh/apache/cloudstack/commit/5a67f1930ccbd2f568c07df53f9a6981ad72f629?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`20f5556`](https://app.codecov.io/gh/apache/cloudstack/commit/20f5556b2044b882e8824281fc19bb93dc9b12c6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
   :warning: Report is 1 commits behind head on main.
   > :exclamation:  There is a different number of reports uploaded between BASE (5a67f19) and HEAD (20f5556). Click for more details.
   > 
   > <details><summary>HEAD has 1 upload less than BASE</summary>
   >
   >| Flag | BASE (5a67f19) | HEAD (20f5556) |
   >|------|------|------|
   >|unittests|1|0|
   ></details>
   
   <details><summary>Additional details and impacted files</summary>
   
   
   
   ```diff
   @@              Coverage Diff              @@
   ##               main   #13828       +/-   ##
   =============================================
   - Coverage     19.64%    3.41%   -16.24%     
   =============================================
     Files          6368      487     -5881     
     Lines        574889    41867   -533022     
     Branches      70353     7912    -62441     
   =============================================
   - Hits         112962     1429   -111533     
   + Misses       449656    40238   -409418     
   + Partials      12271      200    -12071     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/13828/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/13828/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `3.41% <ø> (ø)` | |
   | [unittests](https://app.codecov.io/gh/apache/cloudstack/pull/13828/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   </details>
   
   [:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/cloudstack/pull/13828?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).   
   :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   <details><summary> :rocket: New features to boost your workflow: </summary>
   
   - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
   - :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
   </details>


-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] API key pair restructure follow-ups [cloudstack]

Posted by "bernardodemarco (via GitHub)" <gi...@apache.org>.
bernardodemarco commented on PR #13828:
URL: https://github.com/apache/cloudstack/pull/13828#issuecomment-5296654373

   @blueorangutan package


-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] API key pair restructure follow-ups [cloudstack]

Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13828:
URL: https://github.com/apache/cloudstack/pull/13828#issuecomment-5328454251

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


-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] API key pair restructure follow-ups [cloudstack]

Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13828:
URL: https://github.com/apache/cloudstack/pull/13828#issuecomment-5226666151

   @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.


-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] API key pair restructure follow-ups [cloudstack]

Posted by "bernardodemarco (via GitHub)" <gi...@apache.org>.
bernardodemarco commented on PR #13828:
URL: https://github.com/apache/cloudstack/pull/13828#issuecomment-5327909778

   @blueorangutan package


-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] API key pair restructure follow-ups [cloudstack]

Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13828:
URL: https://github.com/apache/cloudstack/pull/13828#issuecomment-5322137094

   <b>[SF] Trillian test result (tid-16758)</b>
   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
   --- | --- | --- | ---
   


-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] API key pair restructure follow-ups [cloudstack]

Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13828:
URL: https://github.com/apache/cloudstack/pull/13828#issuecomment-5297065238

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


-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] API key pair restructure follow-ups [cloudstack]

Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on code in PR #13828:
URL: https://github.com/apache/cloudstack/pull/13828#discussion_r3830931539


##########
server/src/main/java/com/cloud/api/ApiServer.java:
##########
@@ -1156,7 +1156,7 @@ public boolean verifyRequest(final Map<String, Object[]> requestParameters, fina
             CallContext.register(user, account);
 
             List<ApiKeyPairPermission> keyPairPermissions = keyPairManager.findAllPermissionsByKeyPairId(keyPair.getId(), account.getRoleId());
-            if (commandAvailable(remoteAddress, commandName, user, keyPairPermissions.toArray(new ApiKeyPairPermission[0]))) {
+            if (commandAvailable(remoteAddress, commandName, user, keyPair, keyPairPermissions.toArray(new ApiKeyPairPermission[0]))) {
                 logger.info("API accessed through API Key Pair. API Key: [{}].", keyPair.getApiKey());

Review Comment:
   I think this should be moved from line 1156 to line 1160, right ?
   ```
               CallContext.register(user, account);
   ```
   
   cc @bernardodemarco @winterhazel 
   



-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] API key pair restructure follow-ups [cloudstack]

Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13828:
URL: https://github.com/apache/cloudstack/pull/13828#issuecomment-5296658809

   @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.


-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] API key pair restructure follow-ups [cloudstack]

Posted by "bernardodemarco (via GitHub)" <gi...@apache.org>.
bernardodemarco commented on code in PR #13828:
URL: https://github.com/apache/cloudstack/pull/13828#discussion_r3786079325


##########
server/src/main/java/com/cloud/user/AccountManagerImpl.java:
##########
@@ -3274,17 +3277,17 @@ public ListResponse<ApiKeyPairResponse> listKeys(ListUserKeysCmd cmd) {
         List<ApiKeyPairResponse> responses = new ArrayList<>();
 
         if (cmd.getKeyId() != null || cmd.getApiKeyFilter() != null) {
-            fetchOnlyOneKeyPair(responses, cmd);
+            populateSingleKeyPairResponse(responses, cmd);
             finalResponse.setResponses(responses);
             return finalResponse;
         }
 
-        Integer total = fetchMultipleKeyPairs(responses, cmd);
-        finalResponse.setResponses(responses, total);
+        populateMultipleKeyPairsResponse(responses, cmd);
+        finalResponse.setResponses(responses);

Review Comment:
   Yes, it breaks... reverted that commit



-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] API key pair restructure follow-ups [cloudstack]

Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #13828:
URL: https://github.com/apache/cloudstack/pull/13828#issuecomment-5328992395

   
   @blueorangutan test
   


-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] API key pair restructure follow-ups [cloudstack]

Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13828:
URL: https://github.com/apache/cloudstack/pull/13828#issuecomment-5226851302

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


-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] API key pair restructure follow-ups [cloudstack]

Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13828:
URL: https://github.com/apache/cloudstack/pull/13828#issuecomment-5327920618

   @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.


-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] API key pair restructure follow-ups [cloudstack]

Posted by "winterhazel (via GitHub)" <gi...@apache.org>.
winterhazel merged PR #13828:
URL: https://github.com/apache/cloudstack/pull/13828


-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] API key pair restructure follow-ups [cloudstack]

Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13828:
URL: https://github.com/apache/cloudstack/pull/13828#issuecomment-5338422035

   <b>[SF] Trillian test result (tid-16776)</b>
   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
   


-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] API key pair restructure follow-ups [cloudstack]

Posted by "wido (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] API key pair restructure follow-ups [cloudstack]

Posted by "bernardodemarco (via GitHub)" <gi...@apache.org>.
bernardodemarco commented on code in PR #13828:
URL: https://github.com/apache/cloudstack/pull/13828#discussion_r3832330811


##########
server/src/main/java/com/cloud/api/ApiServer.java:
##########
@@ -1156,7 +1156,7 @@ public boolean verifyRequest(final Map<String, Object[]> requestParameters, fina
             CallContext.register(user, account);
 
             List<ApiKeyPairPermission> keyPairPermissions = keyPairManager.findAllPermissionsByKeyPairId(keyPair.getId(), account.getRoleId());
-            if (commandAvailable(remoteAddress, commandName, user, keyPairPermissions.toArray(new ApiKeyPairPermission[0]))) {
+            if (commandAvailable(remoteAddress, commandName, user, keyPair, keyPairPermissions.toArray(new ApiKeyPairPermission[0]))) {
                 logger.info("API accessed through API Key Pair. API Key: [{}].", keyPair.getApiKey());

Review Comment:
   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.



-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org