Fixing listVirtualMachinesMetrics to extend ListVMsCmd instead of ListVMsCmdByAdmin by davidjumani · Pull Request #4145 · apache/cloudstack

davidjumani · 2020-06-15T06:12:22Z

Description

This fixes the issue of listVirtualMachinesMetrics to extend ListVMsCmd instead of ListVMsCmdByAdmin which is a potential security bug. It now only extends ListVMsCmd, so only admins can see additional details such as internal instance name, etc
No similar issue for listVolumeMetics, listStoragePoolMetrics, etc
Fixes: #4143

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)

…tVMsCmdByAdmin

davidjumani · 2020-06-15T06:12:57Z

@blueorangutan package

blueorangutan · 2020-06-15T06:13:21Z

@davidjumani a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2020-06-15T06:38:38Z

Packaging result: ✔centos7 ✔debian. JID-1377

davidjumani · 2020-06-15T07:32:22Z

@blueorangutan test

blueorangutan · 2020-06-15T07:33:26Z

@davidjumani a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

weizhouapache · 2020-06-15T08:07:21Z

server/src/main/java/com/cloud/api/query/QueryManagerImpl.java

        ResponseView respView = ResponseView.Restricted;
-        if (cmd instanceof ListVMsCmdByAdmin) {
+        Account caller = CallContext.current().getCallingAccount();
+        if (_accountMgr.isAdmin(caller.getId())) {


@davidjumani it should be isRootAdmin instead of isAdmin

@weizhouapache Does it detect ListVMsCmd as ListVMsCmdByAdmin for a regular admin ?

@davidjumani yes, root admin. Check if isAdmin checks for only root admin, or would it pass for domain admin. The fix needed here is to check if caller is a root admin.

Nevermind, I checked and made that fix.

blueorangutan · 2020-06-15T18:30:32Z

Trillian test result (tid-1727)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 37511 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4145-t1727-kvm-centos7.zip
Smoke tests completed. 83 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

yadvr · 2020-06-16T04:36:32Z

@blueorangutan package

blueorangutan · 2020-06-16T04:37:25Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2020-06-16T05:32:35Z

Packaging result: ✔centos7 ✔debian. JID-1380

yadvr · 2020-06-16T05:34:00Z

@blueorangutan test

blueorangutan · 2020-06-16T05:34:30Z

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2020-06-16T17:11:07Z

Trillian test result (tid-1734)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 39861 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4145-t1734-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_internal_lb.py
Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermittent failure detected: /marvin/tests/smoke/test_volumes.py
Smoke tests completed. 75 look OK, 2 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_01_internallb_roundrobin_1VPC_3VM_HTTP_port80	`Failure`	3668.56	test_internal_lb.py
test_01_internallb_roundrobin_1VPC_3VM_HTTP_port80	`Error`	3670.09	test_internal_lb.py
test_02_vpc_privategw_static_routes	`Failure`	189.23	test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	207.79	test_privategw_acl.py
test_04_rvpc_privategw_static_routes	`Failure`	345.35	test_privategw_acl.py

yadvr · 2020-06-17T01:42:30Z

@blueorangutan test

blueorangutan · 2020-06-17T01:43:34Z

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

yadvr · 2020-06-18T00:22:22Z

@blueorangutan test

blueorangutan · 2020-06-18T00:23:38Z

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

shwstppr

Change works. LGTM if domain admins are not allowed see VM's internal name

blueorangutan · 2020-06-18T13:33:58Z

Trillian test result (tid-1759)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 31354 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4145-t1759-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Smoke tests completed. 76 look OK, 1 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_02_vpc_privategw_static_routes	`Failure`	222.90	test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	267.48	test_privategw_acl.py
test_04_rvpc_privategw_static_routes	`Failure`	249.62	test_privategw_acl.py

Fixing listVirtualMachinesMetrics to extend ListVMsCmd instead of Lis…

902755c

…tVMsCmdByAdmin

davidjumani changed the base branch from master to 4.13 June 15, 2020 07:39

weizhouapache reviewed Jun 15, 2020

View reviewed changes

yadvr added this to the 4.13.2.0 milestone Jun 16, 2020

yadvr added 2 commits June 16, 2020 09:58

Update QueryManagerImpl.java

a0d194e

clusterid/pod/storage pool etc should not be returned to domain admin

cab6a07

yadvr requested review from weizhouapache and yadvr June 16, 2020 04:35

yadvr approved these changes Jun 16, 2020

View reviewed changes

yadvr added component:api type:bug labels Jun 16, 2020

yadvr requested review from DaanHoogland, GabrielBrascher, harikrishna-patnala and nvazquez June 17, 2020 01:44

shwstppr approved these changes Jun 18, 2020

View reviewed changes

apache deleted a comment from blueorangutan Jun 18, 2020

yadvr merged commit 06f3ff0 into apache:4.13 Jun 18, 2020

DaanHoogland added the Severity:Minor label Nov 10, 2020

Conversation

davidjumani commented Jun 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Uh oh!

davidjumani commented Jun 15, 2020

Uh oh!

blueorangutan commented Jun 15, 2020

Uh oh!

blueorangutan commented Jun 15, 2020

Uh oh!

davidjumani commented Jun 15, 2020

Uh oh!

blueorangutan commented Jun 15, 2020

Uh oh!

weizhouapache Jun 15, 2020

Choose a reason for hiding this comment

Uh oh!

davidjumani Jun 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yadvr Jun 16, 2020

Choose a reason for hiding this comment

Uh oh!

yadvr Jun 16, 2020

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Jun 15, 2020

Uh oh!

yadvr commented Jun 16, 2020

Uh oh!

blueorangutan commented Jun 16, 2020

Uh oh!

blueorangutan commented Jun 16, 2020

Uh oh!

yadvr commented Jun 16, 2020

Uh oh!

blueorangutan commented Jun 16, 2020

Uh oh!

blueorangutan commented Jun 16, 2020

Uh oh!

yadvr commented Jun 17, 2020

Uh oh!

blueorangutan commented Jun 17, 2020

Uh oh!

yadvr commented Jun 18, 2020

Uh oh!

blueorangutan commented Jun 18, 2020

Uh oh!

shwstppr left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Jun 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

davidjumani commented Jun 15, 2020 •

edited

Loading

davidjumani Jun 15, 2020 •

edited

Loading