Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Api keypair restructure #9504

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

KlausDornsbach
Copy link
Contributor

@KlausDornsbach KlausDornsbach commented Aug 8, 2024

Description

API access keypairs are primarily used to support interactions between systems, without the need to create sessions (through user and password authentication). Currently, CloudStack's implementation of API keypairs does not allow you to specify permissions for each keypair, simply using the account's default permissions. Additionally, the number of keypairs is limited to one per user and they have no start and end dates.

An extension of the API keypairs functionality was implemented, adding several new features that increase flexibility and security. It is now possible to specify a subset of permissions (from the base account) for each keypair, as well as create more than one key per user. It is also possible to define start and end dates for the validity of a keypair. A key created without an expiration date will always be valid up until it is deleted. It should be noted that creating API keypairs without specifying permissions just creates an API keypair with all account's base permissions. Also, API keypairs older than this patch will always be viewed as keypairs with full account permissions.

The following endpoints were created:

A new listUserKeys API was added. Through this API the user will be able to specify a single keypairid to fetch its specific properties, or apikeyfilter to return a specific keypair based on an apikey. The user can inform an userid to fetch an user's api keypair list. If no keypairid, apikeyfilter or userid is provided, the API defaults to fetching information on the calling user. The listall property allows for fetching all keypairs in the structure that are visible based on the calling user/keypair permissions, if not specified, it defaults to false, fetching only the apikeys on the level of the calling user/keypair. Also, it is possible to inform showpermissions to list all permissions associated with each returned apikey.

Name Description Required Default
userid id of the owner of the keypairs no none
keypairid id of the keypair no none
listall list all accessable keypairs no false
apikeyfilter apikey of they keypair no none
showpermissions lists all associated apikey permissions no false

The API getUserKeys was modified preserving backwards compatibility. It now fetches the last keypair created for the informed user.

The api registerUserKeys was modified so the new API keypair parameters could be specified on creation:

Name Description Required Example Default
id user id Yes b8914774-771e-11ee-8e59-5254003754dc none
name name of the keypair No MyKey userId + " - API Keypair"
startdate date when key becomes valid No 2024-04-09 none
enddate date when key expires No 2024-04-09 never expires
description keypair description No read only permissions none
rules list of API access rules No rules[1].rule=list* rules[1].permission=allow all user API permissions based on Account Role

A new keypair deletion API was added (deleteUserKeys). It will accept only one required argument, the keypair id.

Name Description Required
keypairid id of the keypair yes

I also added a listUserKeyRules api, allowing the user to list the rules associated with an API keypair.

Name Description Required
keypairid id of the keypair yes

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

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

How Has This Been Tested?

API Key Creation and Basic Testing

Single Key (via UI):

  1. I was able to create API keypairs through the UI through the button on the top right of the user view;
  2. Through Cloud Monkey, validated that the keypair had the same permissions as the base user by calling a series of APIs;

Multiple Keys (via Cloud Monkey):

  1. Created multiple keys;
  2. Validated the operation was successful on the DB;
  3. Tested creating a key that was not valid and would become valid in the future, with success;
  4. Tested creating a key that was valid and would become invalid in the future, with success;
  5. Tested trying to create keys that were already invalid and got errors;
  6. All permissions of the API key pairs were consistent with each key pair tested.

Permissions Validation

Tested the permissions of keyrules listing, keypair listing, keypair deletion and keypair cretion with the following user/account/domain setup:

  • domain /ROOT
    • account root admin

      • user root admin
      • user user1
    • domain subdomain

      • account domain admin
        • user domain admin
      • account userAccount
        • user user2
        • user user3

The following table describes the results obtained when the user on the first column attempted to operate on the keypairs of users on the first row (V: operation was possible, F: operation was not possible).

user rootAdm user1 domainAdm user2 user3
rootAdm V V V V V
user1 V V V V V
domainAdm F F V V V
user2 F F F V F
user3 F F F F V

Migration to api_keypair Table

  1. A key was created for a read-only user
  2. The database was updated from version 4.19 to 4.20
  3. The API key data was successfully migrated to the api_keypair table, with the corresponding columns in the user table deleted.
  4. Confirmed correct permission handling of the api key.

General validations

  • Could not create an API keypair with more permissions than the base keypair.
  • Deleting system keys was not possible.
  • After deleting a user or account, the API keypairs were invalidated.

Copy link

codecov bot commented Aug 12, 2024

Codecov Report

Attention: Patch coverage is 15.63071% with 923 lines in your changes missing coverage. Please review.

Project coverage is 15.78%. Comparing base (46201ee) to head (f5efd4e).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
...c/main/java/com/cloud/user/AccountManagerImpl.java 30.33% 177 Missing and 9 partials ⚠️
...he/cloudstack/api/response/ApiKeyPairResponse.java 0.00% 127 Missing ⚠️
...n/java/org/apache/cloudstack/acl/ApiKeyPairVO.java 26.77% 92 Missing and 1 partial ⚠️
...ck/api/command/admin/user/RegisterUserKeysCmd.java 0.00% 80 Missing ⚠️
...src/main/java/com/cloud/api/ApiResponseHelper.java 0.00% 66 Missing ⚠️
...g/apache/cloudstack/acl/dao/ApiKeyPairDaoImpl.java 0.00% 64 Missing ⚠️
...ava/com/cloud/upgrade/dao/Upgrade41910to42000.java 2.63% 37 Missing ⚠️
.../cloudstack/discovery/ApiDiscoveryServiceImpl.java 12.82% 32 Missing and 2 partials ⚠️
...dstack/api/command/admin/user/ListUserKeysCmd.java 0.00% 31 Missing ⚠️
...tack/api/command/admin/user/DeleteUserKeysCmd.java 0.00% 27 Missing ⚠️
... and 21 more
Additional details and impacted files
@@            Coverage Diff             @@
##               main    #9504    +/-   ##
==========================================
  Coverage     15.78%   15.78%            
- Complexity    12565    12578    +13     
==========================================
  Files          5627     5637    +10     
  Lines        492260   493125   +865     
  Branches      63882    63301   -581     
==========================================
+ Hits          77710    77857   +147     
- Misses       406076   406781   +705     
- Partials       8474     8487    +13     
Flag Coverage Δ
uitests 4.04% <ø> (ø)
unittests 16.60% <15.63%> (+<0.01%) ⬆️

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

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

@DaanHoogland
Copy link
Contributor

nice feature @KlausDornsbach , some suggestions,

  1. would it make sense to be able to delete a kay based on name?
  2. also is admin allowed to delete keys for other users? (would make sense from a maintainance point of view, would it?)

@KlausDornsbach
Copy link
Contributor Author

Hey @DaanHoogland, thanks for taking a look!

It would make sense to be able to delete a keypair by name, we would just need to block users from creating multiple API keypairs with the same name.

At the moment an admin is allowed to delete keypairs from users it has access, for example, a root admin user could delete any keypair in the platform, domain admin users can delete any keypair in the domain, normal users can only delete their own keys. These permissions are also true for visualization and creation APIs.

@DaanHoogland
Copy link
Contributor

It would make sense to be able to delete a keypair by name, we would just need to block users from creating multiple API keypairs with the same name.

Well, I think a unique constraint on UserId/KeyPairName makes sense also from a usability sense.

At the moment an admin is allowed to delete keypairs from users it has access, for example, a root admin user could delete any keypair in the platform, domain admin users can delete any keypair in the domain, normal users can only delete their own keys. These permissions are also true for visualization and creation APIs.

👍

@rajujith rajujith self-assigned this Aug 19, 2024
@rajujith
Copy link
Collaborator

@blueorangutan package

Copy link

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

@apache apache deleted a comment from blueorangutan Aug 20, 2024
@apache apache deleted a comment from blueorangutan Aug 20, 2024
@apache apache deleted a comment from blueorangutan Aug 20, 2024
@blueorangutan
Copy link

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

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

@DaanHoogland
Copy link
Contributor

@blueorangutan test

@blueorangutan
Copy link

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

@blueorangutan
Copy link

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

Test Result Time (s) Test File
ContextSuite context=TestISOUsage>:setup Error 0.00 test_usage.py
test_01_secure_vm_migration Error 133.28 test_vm_life_cycle.py
test_01_secure_vm_migration Error 133.29 test_vm_life_cycle.py
ContextSuite context=TestVmSnapshot>:setup Error 1910.14 test_vm_snapshots.py
test_01_cancel_host_maintenace_with_no_migration_jobs Error 29.68 test_host_maintenance.py
test_02_cancel_host_maintenace_with_migration_jobs Error 164.07 test_host_maintenance.py
test_01_redundant_vpc_site2site_vpn Failure 467.13 test_vpc_vpn.py
all_test_vm_strict_host_tags Skipped --- test_vm_strict_host_tags.py

@KlausDornsbach
Copy link
Contributor Author

Hey guys, looking at the Marvin test logs I have found the following error

n3.6/unittest/case.py", line 60, in testPartExecutor
    yield
  File "/usr/lib64/python3.6/unittest/case.py", line 622, in run
    testMethod()
  File "/marvin/tests/smoke/test_accounts.py", line 1711, in test_user_key_renew_same_account
    new_keys = User.registerUserKeys(cs_api, user_1.id)
  File "/usr/local/lib/python3.6/site-packages/marvin/lib/base.py", line 338, in registerUserKeys
    return apiclient.registerUserKeys(cmd)
  File "/usr/local/lib/python3.6/site-packages/marvin/cloudstackAPI/cloudstackAPIClient.py", line 2215, in registerUserKeys
    response = self.connection.marvinRequest(command, response_type=response, method=method)
  File "/usr/local/lib/python3.6/site-packages/marvin/cloudstackConnection.py", line 381, in marvinRequest
    raise e
  File "/usr/local/lib/python3.6/site-packages/marvin/cloudstackConnection.py", line 376, in marvinRequest
    raise self.__lastError
  File "/usr/local/lib/python3.6/site-packages/marvin/cloudstackConnection.py", line 310, in __parseAndGetResponse
    response_cls)
  File "/usr/local/lib/python3.6/site-packages/marvin/jsonHelper.py", line 155, in getResultObj
    raise cloudstackException.CloudstackAPIException(respname, errMsg)
Execute cmd: registeruserkeys failed, due to: errorCode: 531, errorText:Only admins can operate on API keys owned by other users

Which indicates the API is outputting an error with the message Only admins can operate on API keys owned by other users. This was removed in the following commit fixing marvin test and there are no more occurrences of this specific error in the code.

is it possible blueorangutang is basing tests on an older commit?

@DaanHoogland
Copy link
Contributor

is it possible blueorangutang is basing tests on an older commit?

by the looks of the history of comments in PR, I'd say no, @KlausDornsbach . Let me redo it once

@DaanHoogland
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

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

@DaanHoogland
Copy link
Contributor

@blueorangutan test

@blueorangutan
Copy link

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

@blueorangutan
Copy link

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

Test Result Time (s) Test File
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL Error 690.83 test_vpc_redundant.py
test_02_redundant_VPC_default_routes Error 61.49 test_vpc_redundant.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers Error 34.20 test_vpc_redundant.py
test_04_rvpc_network_garbage_collector_nics Error 53.01 test_vpc_redundant.py
test_05_rvpc_multi_tiers Error 87.27 test_vpc_redundant.py
ContextSuite context=TestVPCRedundancy>:teardown Error 129.80 test_vpc_redundant.py
all_test_vm_deployment_planner Skipped --- test_vm_deployment_planner.py
all_test_vm_life_cycle Skipped --- test_vm_life_cycle.py
all_test_vm_lifecycle_unmanage_import Skipped --- test_vm_lifecycle_unmanage_import.py
all_test_vm_schedule Skipped --- test_vm_schedule.py
all_test_vm_snapshots Skipped --- test_vm_snapshots.py
all_test_vpc_router_nics Skipped --- test_vpc_router_nics.py
all_test_vpc_vpn Skipped --- test_vpc_vpn.py
all_test_webhook_delivery Skipped --- test_webhook_delivery.py
all_test_webhook_lifecycle Skipped --- test_webhook_lifecycle.py
all_test_host_maintenance Skipped --- test_host_maintenance.py
all_test_hostha_kvm Skipped --- test_hostha_kvm.py

Copy link
Collaborator

@rajujith rajujith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KlausDornsbach

Creating multiple key pairs works fine
Deletion of keypair works fine
registerUserKeys creates keypair in disabled state, is this expected? How to change the state to enabled?
Able to set validity
Is there a way to update user keys, such as updating the enddate, rules etc.. ?
Creating rule works fine in general but rule with asterisk fails.

(PR-9504) 🐱 > register userkeys id=55cde672-3fd4-4670-95f2-4a88513d9d05 name=jithin-test-rules description=jithin-test-rules startdate=2024-10-08 enddate=2024-10-10 rules[1].rule=list* rules[1].permission=allow
🙈 Error: (HTTP 431, error code 4350) API list* was not found on the user's account, a valid keypair rule must be also present at the account level



Copy link

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

@rajujith
Copy link
Collaborator

@blueorangutan package

@blueorangutan
Copy link

@rajujith a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 11389

@DaanHoogland
Copy link
Contributor

@KlausDornsbach , can you have a look at these?

05:51:43 [ERROR] removeRolesIfNeededTestRoleWithDifferentPermissionsRemoveRoles(org.apache.cloudstack.acl.RoleManagerImplTest)  Time elapsed: 1.399 s  <<< FAILURE!
05:51:43 java.lang.AssertionError: expected:<1> but was:<2>
05:51:43 	at org.apache.cloudstack.acl.RoleManagerImplTest.removeRolesIfNeededTestRoleWithDifferentPermissionsRemoveRoles(RoleManagerImplTest.java:272)
05:51:43 
05:51:43 [ERROR] roleHasPermissionTestRolePermissionsDeniedApiContainRoleToAccessAllowedApiReturnsFalse(org.apache.cloudstack.acl.RoleManagerImplTest)  Time elapsed: 0.001 s  <<< FAILURE!
05:51:43 java.lang.AssertionError
05:51:43 	at org.apache.cloudstack.acl.RoleManagerImplTest.roleHasPermissionTestRolePermissionsDeniedApiContainRoleToAccessAllowedApiReturnsFalse(RoleManagerImplTest.java:306)
05:51:43 
05:51:43 [ERROR] removeRolesIfNeededTestRoleWithLessPermissionsRemoveRoles(org.apache.cloudstack.acl.RoleManagerImplTest)  Time elapsed: 0.001 s  <<< FAILURE!
05:51:43 java.lang.AssertionError: expected:<1> but was:<2>
05:51:43 	at org.apache.cloudstack.acl.RoleManagerImplTest.removeRolesIfNeededTestRoleWithLessPermissionsRemoveRoles(RoleManagerImplTest.java:255)
05:51:43 
05:51:43 [INFO] Running org.apache.cloudstack.annotation.AnnotationManagerImplTest
05:51:43 [INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.505 s - in org.apache.cloudstack.annotation.AnnotationManagerImplTest
05:51:43 [INFO] Running org.apache.cloudstack.privategw.AclOnPrivateGwTest
05:51:43 [INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.616 s - in org.apache.cloudstack.privategw.AclOnPrivateGwTest
05:51:44 [INFO] Running org.apache.cloudstack.user.UserPasswordResetManagerImplTest
05:51:44 [INFO] Tests run: 5, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.415 s - in org.apache.cloudstack.user.UserPasswordResetManagerImplTest
05:51:44 [INFO] Running org.apache.cloudstack.affinity.AffinityApiUnitTest
05:51:44 [INFO] Tests run: 7, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.092 s - in org.apache.cloudstack.affinity.AffinityApiUnitTest
05:51:44 [INFO] Running org.apache.cloudstack.affinity.AffinityGroupServiceImplTest
05:51:44 [INFO] Tests run: 12, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.24 s - in org.apache.cloudstack.affinity.AffinityGroupServiceImplTest
05:51:44 [INFO] 
05:51:45 [INFO] Results:
05:51:45 [INFO] 
05:51:45 [ERROR] Failures: 
05:51:45 [ERROR]   RoleManagerImplTest.removeRolesIfNeededTestRoleWithDifferentPermissionsRemoveRoles:272 expected:<1> but was:<2>
05:51:45 [ERROR]   RoleManagerImplTest.removeRolesIfNeededTestRoleWithLessPermissionsRemoveRoles:255 expected:<1> but was:<2>
05:51:45 [ERROR]   RoleManagerImplTest.roleHasPermissionTestRolePermissionsDeniedApiContainRoleToAccessAllowedApiReturnsFalse:306
05:51:45 [ERROR] Errors: 
05:51:45 [ERROR]   AccountManagerImplTest.createApiAndSecretKeyTestWithNonEmptyDates:1363 ? InvalidParameterValue
05:51:45 [ERROR]   AccountManagerImplTest.createApiKeyAndSecretKeyTestAllowedOnAccount:1323 ? InvalidParameterValue

It seems there are some unit test failures.

@KlausDornsbach
Copy link
Contributor Author

Hey guys, I have fixed the tests. Also, answering the first question @rajujith

  • I could not reproduce the issue of registerUserKeys creating keypairs in the disabled state, could you send the parameters used in the creation?
  • Also, currently there is no way to update the user keys.
  • I have fixed the problem when specifying apis with asterisk.

@DaanHoogland,
Analyzing the integration tests, I think the problem could be in the internal structure running the tests for the following reasons:

  • Latests Marvin logs indicate some problem with all clusters being in avoid set, which I think is not related to this PR:
2024-10-01 07:41:41,367 DEBUG [c.c.d.FirstFitPlanner] (Work-Job-Executor-129:[ctx-39be9a11, job-4383/job-4386, ctx-a2bb1e92]) (logid:919ef11a) Searching all possible resources under this Zone: 1
2024-10-01 07:41:41,407 DEBUG [c.c.d.FirstFitPlanner] (Work-Job-Executor-129:[ctx-39be9a11, job-4383/job-4386, ctx-a2bb1e92]) (logid:919ef11a) Listing clusters in order of aggregate capacity, that have (at least one host with) enough CPU and RAM capacity under this Zone: 1
2024-10-01 07:41:41,477 DEBUG [c.c.d.FirstFitPlanner] (Work-Job-Executor-129:[ctx-39be9a11, job-4383/job-4386, ctx-a2bb1e92]) (logid:919ef11a) Removing from the clusterId list these clusters from avoid set: [1]
2024-10-01 07:41:41,563 DEBUG [c.c.d.FirstFitPlanner] (Work-Job-Executor-129:[ctx-39be9a11, job-4383/job-4386, ctx-a2bb1e92]) (logid:919ef11a) No clusters found after removing disabled clusters and clusters in avoid list, returning.
2024-10-01 07:41:48,742 DEBUG [c.c.c.CapacityManagerImpl] (Work-Job-Executor-129:[ctx-39be9a11, job-4383/job-4386, ctx-a2bb1e92]) (logid:919ef11a) VM instance {"id":435,"instanceName":"r-435-VM","type":"DomainRouter","uuid":"a7eb046b-c2d9-46cd-9220-dae09781535f"} state transited from [Starting] to [Stopped] with event [OperationFailed]. VM's original host: null, new host: null, host before state transition: null
2024-10-01 07:41:51,921 ERROR [c.c.v.VmWorkJobHandlerProxy] (Work-Job-Executor-129:[ctx-39be9a11, job-4383/job-4386, ctx-a2bb1e92]) (logid:919ef11a) Invocation exception, caused by: com.cloud.exception.InsufficientServerCapacityException: Unable to create a deployment for VM instance {"id":435,"instanceName":"r-435-VM","type":"DomainRouter","uuid":"a7eb046b-c2d9-46cd-9220-dae09781535f"}Scope=interface com.cloud.dc.DataCenter; id=1
2024-10-01 07:41:52,458 INFO  [c.c.v.VmWorkJobHandlerProxy] (Work-Job-Executor-129:[ctx-39be9a11, job-4383/job-4386, ctx-a2bb1e92]) (logid:919ef11a) Rethrow exception com.cloud.exception.InsufficientServerCapacityException: Unable to create a deployment for VM instance {"id":435,"instanceName":"r-435-VM","type":"DomainRouter","uuid":"a7eb046b-c2d9-46cd-9220-dae09781535f"}Scope=interface com.cloud.dc.DataCenter; id=1

@rajujith
Copy link
Collaborator

@blueorangutan package

@blueorangutan
Copy link

@rajujith a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants