Disable API Key Access for users, accounts and domains #9741

abh1sar · 2024-09-27T13:05:11Z

Description

This PR implements the feature which give Root Admin the ability to Disable Api-key/Secret-key access at different granularities (User/Account/Domain/Global)
Spec : https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=323488155

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

Screenshots (if appropriate):

How Has This Been Tested?

Local value should always take precedence unless it is set to Inherit.
Tested the following matrix. Result denotes if Api key access was allowed for the User or not.

User	Account	Domain	Global	Result
Inherit	Inherit	Inherit	Enabled	Enabled
Inherit	Inherit	Inherit	Disabled	Disabled
Inherit	Inherit	Enabled	Disabled	Enabled
Inherit	Disabled	Enabled	Enabled	Disabled
Disabled	Enabled	Enabled	Enabled	Disabled
Enabled	Inherit	Inherit	Disabled	Enabled

Tested that apikeyaccess parameter in updateUser, updateAccount, listUsers and listAccounts is not shown to anyone else apart from the Root Admin.
Tested that api.key.access configuration is not editable by the domain admin.

How did you try to break this feature and the system with this change?

…l setting

codecov · 2024-09-27T13:16:17Z

Codecov Report

Attention: Patch coverage is 44.52555% with 76 lines in your changes missing coverage. Please review.

Project coverage is 15.82%. Comparing base (c159471) to head (113c700).
Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
...c/main/java/com/cloud/user/dao/AccountDaoImpl.java	0.00%	14 Missing ⚠️
...n/java/org/apache/cloudstack/api/ApiConstants.java	47.05%	9 Missing ⚠️
...ain/java/com/cloud/api/query/QueryManagerImpl.java	60.86%	5 Missing and 4 partials ⚠️
...ne/schema/src/main/java/com/cloud/user/UserVO.java	0.00%	6 Missing ⚠️
...schema/src/main/java/com/cloud/user/AccountVO.java	16.66%	5 Missing ⚠️
server/src/main/java/com/cloud/api/ApiServer.java	76.19%	4 Missing and 1 partial ⚠️
...ck/api/command/admin/account/UpdateAccountCmd.java	0.00%	3 Missing ⚠️
...loudstack/api/command/admin/user/ListUsersCmd.java	0.00%	3 Missing ⚠️
...oudstack/api/command/admin/user/UpdateUserCmd.java	0.00%	3 Missing ⚠️
...tack/api/command/user/account/ListAccountsCmd.java	0.00%	3 Missing ⚠️
... and 7 more

Additional details and impacted files

@@             Coverage Diff              @@
##              main    #9741       +/-   ##
============================================
+ Coverage     4.03%   15.82%   +11.78%     
- Complexity       0    12572    +12572     
============================================
  Files          392     5626     +5234     
  Lines        32157   492174   +460017     
  Branches      5673    60349    +54676     
============================================
+ Hits          1299    77862    +76563     
- Misses       30710   405816   +375106     
- Partials       148     8496     +8348

Flag	Coverage Δ
uitests	`4.03% <ø> (-0.01%)`	⬇️
unittests	`16.64% <44.52%> (?)`

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.

weizhouapache · 2024-09-27T13:19:31Z

server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java

@@ -955,6 +955,11 @@ public Configuration updateConfiguration(final UpdateCfgCmd cmd) throws InvalidP
            category = config.getCategory();
        }

+        if ("System".equals(category) && !_accountMgr.isRootAdmin(caller.getId())) {


maybe define a constant "System" somewhere

weizhouapache · 2024-09-27T13:20:08Z

api/src/main/java/org/apache/cloudstack/api/command/admin/account/UpdateAccountCmd.java

@@ -70,6 +71,9 @@ public class UpdateAccountCmd extends BaseCmd {
    @Parameter(name = ApiConstants.ACCOUNT_DETAILS, type = CommandType.MAP, description = "Details for the account used to store specific parameters")
    private Map details;

+    @Parameter(name = ApiConstants.API_KEY_ACCESS, type = CommandType.STRING, description = "Determines if Api key access for this user is enabled, disabled or inherits the value from its parent, the domain level setting \"api.key.access\"", since = "4.20.1.0", authorized = {RoleType.Admin})


is it targetted 4.20.1 ?

Yes, is that ok?

abh1sar · 2024-09-30T19:02:23Z

@blueorangutan package

blueorangutan · 2024-09-30T19:04:04Z

@abh1sar 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 · 2024-09-30T20:05:55Z

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

abh1sar · 2024-10-01T08:09:47Z

@blueorangutan test

blueorangutan · 2024-10-01T08:12:07Z

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

sureshanaparti · 2024-10-01T12:47:58Z

engine/schema/src/main/resources/META-INF/db/schema-41910to42000.sql

+    ADD COLUMN `api_key_access` boolean;
+
+ALTER TABLE `cloud`.`account`
+    ADD COLUMN `api_key_access` boolean;


use IDEMPOTENT_ADD_COLUMN call to add columns

sureshanaparti · 2024-10-01T12:51:37Z

server/src/main/java/com/cloud/user/AccountManagerImpl.java

+    public static final ConfigKey<Boolean> apiKeyAccess = new ConfigKey<>(ConfigKey.CATEGORY_SYSTEM, Boolean.class,
+            "api.key.access",
+            "true",
+            "Determines whether API (api-key/secret-key) access is allowed or not. Editable only by Root Admin.",


Suggested change

"Determines whether API (api-key/secret-key) access is allowed or not. Editable only by Root Admin.",

"Determines whether API (api-key/secret-key) access is allowed or not.",

This setting is visible to domain admin, but editable only by root admin.
When they try to edit it, the Domain admin gets a generic error : "There was an error saving this setting. Please try again later"
I thought it might get confusing to the domain admin, that's why I added the info about root admin only being able to edit it.
I can remove it if you still think it is not required.

sureshanaparti · 2024-10-01T12:52:36Z

server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java

@@ -955,6 +956,11 @@ public Configuration updateConfiguration(final UpdateCfgCmd cmd) throws InvalidP
            category = config.getCategory();
        }

+        if (CATEGORY_SYSTEM.equals(category) && !_accountMgr.isRootAdmin(caller.getId())) {
+            logger.warn("Only Root Admin is allowed to edit the configuration " + name);
+            throw new CloudRuntimeException("Only Root Admin is allowed to edit this configuration.");


Suggested change

throw new CloudRuntimeException("Only Root Admin is allowed to edit this configuration.");

throw new CloudRuntimeException("You are not allowed to update this configuration.");

better not to indicate in the msg about who has permissions

Same as above.

sureshanaparti · 2024-10-01T12:57:43Z

engine/schema/src/main/resources/META-INF/db/schema-41910to42000.sql

@@ -425,3 +425,9 @@ INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` (uuid, hypervisor_type, hypervi

 CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.vm_instance', 'delete_protection', 'boolean DEFAULT FALSE COMMENT "delete protection for vm" ');
 CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.volumes', 'delete_protection', 'boolean DEFAULT FALSE COMMENT "delete protection for volumes" ');
+


targeted for 4.20.1? add these changes in engine/schema/src/main/resources/META-INF/db/schema-42000to42010.sql

Will create the new schema file once 4.20.0 is cut. Keeping this PR in draft until then

blueorangutan · 2024-10-01T22:35:12Z

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

Test	Result	Time (s)	Test File

abh1sar added 6 commits September 25, 2024 17:17

cli changes to update user/account, list by apikeyaccess, domain leve…

ceed660

…l setting

UI changes for updating user/account and searchfilter in listview

2f276e6

make the api parameters and setting accessible only to root admin

ad4171a

revert changes to ui/package-lock.json

09bf1a0

minor changes to description strings

2869b13

UT for ApiServer and AccountManagerImpl classes

67c9c62

boring-cyborg bot added component:api component:UI labels Sep 27, 2024

abh1sar added this to the 4.20.1.0 milestone Sep 27, 2024

fix pre-commit failure

aadf4cb

weizhouapache reviewed Sep 27, 2024

View reviewed changes

abh1sar and others added 5 commits September 27, 2024 19:41

Merge branch 'main' into apikey

31eb378

Added a constant for the string System

048642d

Merge branch 'apikey' of https://github.com/abh1sar/cloudstack into apik

b7085e6

UT for searchForUsers and searchForAccounts

aa6bc35

Merge branch 'main' into apikey

65dde8e

abh1sar requested review from harikrishna-patnala and weizhouapache September 30, 2024 09:33

abh1sar added 2 commits September 30, 2024 17:31

Fix marvin test error

f736d77

Merge branch 'apikey' of https://github.com/abh1sar/cloudstack into apik

51be0cb

sureshanaparti reviewed Oct 1, 2024

View reviewed changes

sureshanaparti added the status:needs-testing label Oct 3, 2024

Update schema to use idempotent add column

113c700

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable API Key Access for users, accounts and domains #9741

Disable API Key Access for users, accounts and domains #9741

abh1sar commented Sep 27, 2024

codecov bot commented Sep 27, 2024 •

edited

Loading

weizhouapache Sep 27, 2024

abh1sar Sep 28, 2024

weizhouapache Sep 27, 2024

abh1sar Sep 28, 2024

abh1sar commented Sep 30, 2024

blueorangutan commented Sep 30, 2024

blueorangutan commented Sep 30, 2024

abh1sar commented Oct 1, 2024

blueorangutan commented Oct 1, 2024

sureshanaparti Oct 1, 2024

abh1sar Oct 3, 2024

sureshanaparti Oct 1, 2024

abh1sar Oct 3, 2024

sureshanaparti Oct 1, 2024 •

edited

Loading

abh1sar Oct 3, 2024

sureshanaparti Oct 1, 2024

abh1sar Oct 3, 2024

blueorangutan commented Oct 1, 2024

	"Determines whether API (api-key/secret-key) access is allowed or not. Editable only by Root Admin.",
	"Determines whether API (api-key/secret-key) access is allowed or not.",

	throw new CloudRuntimeException("Only Root Admin is allowed to edit this configuration.");
	throw new CloudRuntimeException("You are not allowed to update this configuration.");

		@@ -425,3 +425,9 @@ INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` (uuid, hypervisor_type, hypervi

		CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.vm_instance', 'delete_protection', 'boolean DEFAULT FALSE COMMENT "delete protection for vm" ');
		CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.volumes', 'delete_protection', 'boolean DEFAULT FALSE COMMENT "delete protection for volumes" ');

Disable API Key Access for users, accounts and domains #9741

Are you sure you want to change the base?

Disable API Key Access for users, accounts and domains #9741

Conversation

abh1sar commented Sep 27, 2024

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

codecov bot commented Sep 27, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abh1sar commented Sep 30, 2024

blueorangutan commented Sep 30, 2024

blueorangutan commented Sep 30, 2024

abh1sar commented Oct 1, 2024

blueorangutan commented Oct 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sureshanaparti Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueorangutan commented Oct 1, 2024

codecov bot commented Sep 27, 2024 •

edited

Loading

sureshanaparti Oct 1, 2024 •

edited

Loading